[pve-devel] [PATCH qemu-server v3 0/6] bugzilla #4225 -- improve handling of unavailable ISOs

[pve-devel] [PATCH qemu-server v3 0/6] bugzilla #4225 -- improve handling of unavailable ISOs

[Daniel Herzig]

This patch series addresses bugzilla entry #4225.

Currently VMs refuse to to start if a configured isofile becomes unavailable,
be it a deleted file or an unavailable network storage.

This patch series introduces a new parameter in Drive.pm, called 'essential'.
Depending on whether this parameter is set or not, the situation will be handled
differently.

If the parameter is set to 0, the configuration will temporarily changed to use
'none' as file for the cd drive, which allows qemu to start up the machine.
The configuration is not changed in this process to avoid unexpected behaviour.
Instead a log_warn will be issued.

For transition reasons an unset parameter acts like 'required=1'. In this case
the startup process will die earlier than currently, if the file is missing or
the underlying storage not available.

If a new VM is created from the WebGUI, a corresponding added checkbox
is checked by default, but allows for convenient unchecking during setup time,
eg for media that is only needed for installation time.

This patch series adds an 'Eject' button to the hardwareview in the WebGUI,
which can be used as a convenience shortcut to setting file to 'none' for the
cdrom drive.

This series supersedes:
https://lore.proxmox.com/pve-devel/20250113085608.99498-1-d.herzig@proxmox.com/

Changes since v2:
* rebased onto current masters (qemu-server: 0fc03fc1, pve-manager: bbba1c53)
* refactored patch series structure (THX @Dano)
* shortened code and streamlined code logic (qemu-server, THX @Dano)
* reduced changes on testing framework (qemu-server)
* reuse existing code for parsing values (pve-manager, THX @Friedrich)
* properly html-encode warning messages (pve-manager, THX @Friedrich)


qemu-server: Daniel Herzig (3):
  fix #4225: qemuserver: drive: add parameter to mark drive required
  fix #4225: qemuserver: introduce sub eject_nonrequired_isos
  fix #4225: qemuserver, test: put eject_nonrequired_isos in place

 PVE/QemuServer.pm                             | 37 ++++++++++++++++++
 PVE/QemuServer/Drive.pm                       |  9 ++++-
 test/cfg2cmd/ide-required-iso-missing.conf    | 12 ++++++
 .../cfg2cmd/ide-required-iso-missing.conf.cmd |  0
 .../cfg2cmd/ide-required-iso-offline-nfs.conf | 12 ++++++
 .../ide-required-iso-offline-nfs.conf.cmd     |  0
 test/cfg2cmd/ide-required.conf                | 14 +++++++
 test/cfg2cmd/ide-required.conf.cmd            | 39 +++++++++++++++++++
 test/cfg2cmd/ide-unrequired-iso-missing.conf  | 12 ++++++
 .../ide-unrequired-iso-missing.conf.cmd       | 33 ++++++++++++++++
 .../ide-unrequired-iso-offline-nfs.conf       | 12 ++++++
 .../ide-unrequired-iso-offline-nfs.conf.cmd   | 33 ++++++++++++++++
 test/run_config2command_tests.pl              | 36 +++++++++++++++++
 13 files changed, 248 insertions(+), 1 deletion(-)
 create mode 100644 test/cfg2cmd/ide-required-iso-missing.conf
 create mode 100644 test/cfg2cmd/ide-required-iso-missing.conf.cmd
 create mode 100644 test/cfg2cmd/ide-required-iso-offline-nfs.conf
 create mode 100644 test/cfg2cmd/ide-required-iso-offline-nfs.conf.cmd
 create mode 100644 test/cfg2cmd/ide-required.conf
 create mode 100644 test/cfg2cmd/ide-required.conf.cmd
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-missing.conf
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-missing.conf.cmd
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf.cmd

pve-manager: Daniel Herzig (3):
  fix #4225: ui: form: isoselector: add checkbox for 'essential' param
  fix #4225: ui: qemu: cdedit: enable 'Essential' checkbox for isos
  fix #4225: ui: qemu: hardware: add eject button for cdroms

 www/manager6/form/IsoSelector.js  | 22 ++++++++++++++++
 www/manager6/qemu/CDEdit.js       |  8 ++++++
 www/manager6/qemu/HardwareView.js | 42 +++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)

-- 
2.39.5


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

[pve-devel] [PATCH qemu-server v3 1/6] fix #4225: qemuserver: drive: add parameter to mark drive required

[Daniel Herzig]

This commit add the parameter `essential` to mark a drive as required
for booting the VM.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 PVE/QemuServer/Drive.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 1041c1dd..38136787 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -266,7 +266,14 @@ my %drivedesc_base = (
 	verbose_description => "Mark this locally-managed volume as available on all nodes.\n\nWARNING: This option does not share the volume automatically, it assumes it is shared already!",
 	optional => 1,
 	default => 0,
-    }
+    },
+    essential => {
+	type => 'boolean',
+	description => 'Mark this iso volume as required for booting the VM.',
+	verbose_description => 'If unset or set to 1, and the iso file is unavailable, the VM will not start.\nThis parameter is considered for cdrom iso drives only.',
+	optional => 1,
+	default => 1,
+    },
 );
 
 my %iothread_fmt = ( iothread => {
-- 
2.39.5


_______________________________________________
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 1/6] fix #4225: qemuserver: drive: add parameter to mark drive required

[Fiona Ebner]

The 'qemuserver' prefix in the commit title doesn't add any information
and should not be there. Commit title prefixes are not for file names.
This also doesn't fix the issue yet, so I'd also drop that prefix too.

Am 30.01.25 um 12:31 schrieb Daniel Herzig:
> This commit add the parameter `essential` to mark a drive as required
> for booting the VM.
> 
> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
> ---
>  PVE/QemuServer/Drive.pm | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 1041c1dd..38136787 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -266,7 +266,14 @@ my %drivedesc_base = (
>  	verbose_description => "Mark this locally-managed volume as available on all nodes.\n\nWARNING: This option does not share the volume automatically, it assumes it is shared already!",
>  	optional => 1,
>  	default => 0,
> -    }
> +    },
> +    essential => {
> +	type => 'boolean',
> +	description => 'Mark this iso volume as required for booting the VM.',

Nit: Since the 'media=cdrom' option is used to decide this, I'd state
"CD-ROM" here to be more precise. I'd also say "for starting the VM"
rather than "for booting the VM". The device isn't necessarily involved
into booting, but can still be considered essential to allow starting
the VM.

> +	verbose_description => 'If unset or set to 1, and the iso file is unavailable, the VM will not start.\nThis parameter is considered for cdrom iso drives only.',
> +	optional => 1,
> +	default => 1,
> +    },
>  );
>  
>  my %iothread_fmt = ( iothread => {

There should be some checking/handling in the API endpoints and/or
parse_drive() for making sure this can only be set in combination with
'media=cdrom'. There are already such checks in parse_drive() which will
return undef in those cases, but please also issue a warning for a new
such check so that it will be clear what went wrong ;)

Nit: Usually, it's nicer to have booleans be 0 if not present. So can we
invert this e.g. "ignore-if-missing" (or "detach-if-missing" or
"eject-if-missing")? Otherwise, maybe "essential-for-start" to be more
descriptive?


_______________________________________________
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 1/6] fix #4225: qemuserver: drive: add parameter to mark drive required

[Daniel Herzig]

Fiona Ebner <f.ebner@proxmox.com> writes:

> The 'qemuserver' prefix in the commit title doesn't add any information
> and should not be there. Commit title prefixes are not for file names.
> This also doesn't fix the issue yet, so I'd also drop that prefix too.
>
Right, missed that redundancy. Will be fixed in some v4.

> Am 30.01.25 um 12:31 schrieb Daniel Herzig:
>> This commit add the parameter `essential` to mark a drive as required
>> for booting the VM.
>> 
>> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
>> ---
>>  PVE/QemuServer/Drive.pm | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>> 
>> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
>> index 1041c1dd..38136787 100644
>> --- a/PVE/QemuServer/Drive.pm
>> +++ b/PVE/QemuServer/Drive.pm
>> @@ -266,7 +266,14 @@ my %drivedesc_base = (
>>  	verbose_description => "Mark this locally-managed volume as available on all nodes.\n\nWARNING: This option does not share the volume automatically, it assumes it is shared already!",
>>  	optional => 1,
>>  	default => 0,
>> -    }
>> +    },
>> +    essential => {
>> +	type => 'boolean',
>> +	description => 'Mark this iso volume as required for booting the VM.',
>
> Nit: Since the 'media=cdrom' option is used to decide this, I'd state
> "CD-ROM" here to be more precise. I'd also say "for starting the VM"
> rather than "for booting the VM". The device isn't necessarily involved
> into booting, but can still be considered essential to allow starting
> the VM.

Thanks for pointing this out, this part needs some re-working indeed.

>
>> +	verbose_description => 'If unset or set to 1, and the iso file is unavailable, the VM will not start.\nThis parameter is considered for cdrom iso drives only.',
>> +	optional => 1,
>> +	default => 1,
>> +    },
>>  );
>>  
>>  my %iothread_fmt = ( iothread => {
>
> There should be some checking/handling in the API endpoints and/or
> parse_drive() for making sure this can only be set in combination with
> 'media=cdrom'. There are already such checks in parse_drive() which will
> return undef in those cases, but please also issue a warning for a new
> such check so that it will be clear what went wrong ;)
>
I'll look this up and check against various ~qm set~ scenarios.

> Nit: Usually, it's nicer to have booleans be 0 if not present. So can we
> invert this e.g. "ignore-if-missing" (or "detach-if-missing" or
> "eject-if-missing")? Otherwise, maybe "essential-for-start" to be more
> descriptive?

Good idea, getting ready to invert my brains :)


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

[pve-devel] [PATCH qemu-server v3 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos

[Daniel Herzig]

The function `eject_nonrequired_isos` checks on whether a cdrom drive is
marked as required for booting the VM  or not. If the parameter `essential` is not
defined, it will assume `essential` 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 meant to be called from `config_to_command` while it
iterates over all volumes to allow for early storage activation and
early exit in the case of missing required files or unavailable
network storages.

This commit also adds a small helper sub `file_exists` to facilitate
mocking of file existence, which is needed for testing
`eject_nonrequired_isos` once in place.
---
 PVE/QemuServer.pm | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 808c0e1c..d151c322 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8797,4 +8797,39 @@ sub delete_ifaces_ipams_ips {
     }
 }
 
+sub eject_nonrequired_isos {
+    my ($ds, $drive, $vmid, $storecfg, $conf) = @_;
+
+    # exclude cloudinit drives from processing (parameter '1')
+    return if ( !drive_is_cdrom($drive, 1) ||
+		$drive->{file} eq 'none' ||
+		$drive->{file} eq 'cdrom' );
+
+    $drive->{essential} = 1 if !defined($drive->{essential});
+    my $iso_volid = $drive->{file};
+    my $iso_path = PVE::QemuServer::Drive::get_iso_path($storecfg, $vmid, $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 = $@;
+    }
+
+    return if ( !$store_err &&
+		file_exists($iso_path) );
+
+    if ($drive->{essential}) {
+	die "'${ds}: ${iso_volid}': storage unavailable or file does not exist\n";
+    } else {
+	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
+}
+
 1;
-- 
2.39.5


_______________________________________________
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 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos

[Fiona Ebner]

Am 30.01.25 um 12:31 schrieb Daniel Herzig:
> The function `eject_nonrequired_isos` checks on whether a cdrom drive is
> marked as required for booting the VM  or not. If the parameter `essential` is not
> defined, it will assume `essential` 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 meant to be called from `config_to_command` while it
> iterates over all volumes to allow for early storage activation and
> early exit in the case of missing required files or unavailable
> network storages.
> 
> This commit also adds a small helper sub `file_exists` to facilitate
> mocking of file existence, which is needed for testing
> `eject_nonrequired_isos` once in place.
> ---
>  PVE/QemuServer.pm | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 808c0e1c..d151c322 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -8797,4 +8797,39 @@ sub delete_ifaces_ipams_ips {
>      }
>  }
>  
> +sub eject_nonrequired_isos {
> +    my ($ds, $drive, $vmid, $storecfg, $conf) = @_;
> +
> +    # exclude cloudinit drives from processing (parameter '1')
> +    return if ( !drive_is_cdrom($drive, 1) ||
> +		$drive->{file} eq 'none' ||
> +		$drive->{file} eq 'cdrom' );

Style nit: post-ifs should not be multiline, post-ifs don't use
additional parentheses:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If

> +
> +    $drive->{essential} = 1 if !defined($drive->{essential});

This should rather be done when parsing the drive.

> +    my $iso_volid = $drive->{file};

Nit: variable could be moved up so that the previous check can already
use it

> +    my $iso_path = PVE::QemuServer::Drive::get_iso_path($storecfg, $vmid, $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 = $@;

I'd issue a warning here if there was a failure to activate.

> +    }
> +
> +    return if ( !$store_err &&
> +		file_exists($iso_path) );
> +
> +    if ($drive->{essential}) {
> +	die "'${ds}: ${iso_volid}': storage unavailable or file does not exist\n";
> +    } else {
> +	log_warn("eject '${ds}: ${iso_volid}': storage unavailable or file does not exist");

Technically, this does not eject anything, just changes the configuration.

> +	$drive->{file} = 'none';
> +	$conf->{$ds} = print_drive($drive);

Should we also make sure to write out the modified config? Especially
for live-migration it's important to stay in-sync and so the migration
target should not suddenly have a CD-ROM again if it became accessible
again. Writing the config can also be done by the caller of course if it
better fits there.

> +    }
> +}
> +
> +sub file_exists {
> +    my $file_path = shift;
> +    return -e $file_path

This will only work for file-based storages. The PVE::Storage::path()
function can also return a protocol-based path like "glusterfs://xyz"
which will not work for "-e" checks. (Our glusterfs plugin does so only
for content type "images", but nothing requires a "media=cdrom" drive to
be of content type "iso" right now, and I'm making a general point here,
there are third-party plugins too). So to cleanly solve this, it will be
required to either use e.g. "qemu-img info" as a proxy for existence
checking, which is often done in our code via volume_size_info() (or
alternatively to add a dedicated storage+plugin API function to check
for the existence of an image).

> +}
> +
>  1;



_______________________________________________
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 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos

[Fiona Ebner]

Am 31.01.25 um 10:36 schrieb Fiona Ebner:
> Am 30.01.25 um 12:31 schrieb Daniel Herzig:
>> +
>> +    $drive->{essential} = 1 if !defined($drive->{essential});
> 
> This should rather be done when parsing the drive.

Or I guess to avoid (potentially) writing it out for every drive, it
should only be a local variable here and not set in the drive hash.


_______________________________________________
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 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos

[Daniel Herzig]

Fiona Ebner <f.ebner@proxmox.com> writes:

> Am 31.01.25 um 10:36 schrieb Fiona Ebner:
>> Am 30.01.25 um 12:31 schrieb Daniel Herzig:
>>> +
>>> +    $drive->{essential} = 1 if !defined($drive->{essential});
>> 
>> This should rather be done when parsing the drive.
>
> Or I guess to avoid (potentially) writing it out for every drive, it
> should only be a local variable here and not set in the drive hash.

Thanks, I'll double check on this for v4. But I'd assume the hash to be scoped to
`config_to_command` here, or am I missing something?



_______________________________________________
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 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos

[Fiona Ebner]

Am 31.01.25 um 14:58 schrieb Daniel Herzig:
> Fiona Ebner <f.ebner@proxmox.com> writes:
> 
>> Am 31.01.25 um 10:36 schrieb Fiona Ebner:
>>> Am 30.01.25 um 12:31 schrieb Daniel Herzig:
>>>> +
>>>> +    $drive->{essential} = 1 if !defined($drive->{essential});
>>>
>>> This should rather be done when parsing the drive.
>>
>> Or I guess to avoid (potentially) writing it out for every drive, it
>> should only be a local variable here and not set in the drive hash.
> 
> Thanks, I'll double check on this for v4. But I'd assume the hash to be scoped to
> `config_to_command` here, or am I missing something?
> 

But you don't need the modified version in config_to_command() later,
or? And if yes, that can just check the same way again, i.e. using
default value if not set. If we want to explicitly set the value, that
should happen during parsing the drive string. Most of the time it's
surprising to implicitly modify a passed-in value. Better to either
avoid that or if really necessary, explicitly mention it in the function
description.


_______________________________________________
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 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos

[Daniel Herzig]

Fiona Ebner <f.ebner@proxmox.com> writes:

> Am 31.01.25 um 14:58 schrieb Daniel Herzig:
>> Fiona Ebner <f.ebner@proxmox.com> writes:
>> 
>>> Am 31.01.25 um 10:36 schrieb Fiona Ebner:
>>>> Am 30.01.25 um 12:31 schrieb Daniel Herzig:
>>>>> +
>>>>> +    $drive->{essential} = 1 if !defined($drive->{essential});
>>>>
>>>> This should rather be done when parsing the drive.
>>>
>>> Or I guess to avoid (potentially) writing it out for every drive, it
>>> should only be a local variable here and not set in the drive hash.
>> 
>> Thanks, I'll double check on this for v4. But I'd assume the hash to be scoped to
>> `config_to_command` here, or am I missing something?
>> 
>
> But you don't need the modified version in config_to_command() later,
> or? And if yes, that can just check the same way again, i.e. using
> default value if not set. If we want to explicitly set the value, that
> should happen during parsing the drive string. Most of the time it's
> surprising to implicitly modify a passed-in value. Better to either
> avoid that or if really necessary, explicitly mention it in the function
> description.

No, I do not need the modified version of the VM-config later.

What I'm trying to achieve is a more 'forgiving' behaviour in the case
of accidently server-side-deleted iso file/unavailable server (for whatever reason)
attached to a VM. So this is actually aiming at `qm start`, which
implicitly calls `config_to_command` -- without modifying the existing
VM config at all.

If the parameter 'essential' is set to '0', `config_to_command` would --
in case of file unavailability of the iso file -- generate a kvm startup
command that contains "-drive 'if=none,media=cdrom,[...]" instead of
"-drive 'file=$SOME_PATH_TO_ISO,[..]' when we at this point already know
that $SOME_PATH_TO_ISO is unavailable/non-existent.

The VM-config itself is not changed, as an eg nfs-server might come back
at a point in time later and the user might want to do something with the iso
stored there.

If the parameter 'essential' is unset, or set to '1', the die would
happen before `qm start` leads to an invocation of kvm, as it cannot be
expected to lead to a successful action, if $SOME_PATH_TO_ISO is not
reachable. This would be the exact behaviour as we have it now, just not
letting kvm run into this situation, but detect and exit earlier, while
`config_to_commands` iterates over all volumes via `foreach_volume`
anyway before producing the 'final' kvm command.



_______________________________________________
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 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos

[Fiona Ebner]

Am 03.02.25 um 11:15 schrieb Daniel Herzig:
> Fiona Ebner <f.ebner@proxmox.com> writes:
> 
>> Am 31.01.25 um 14:58 schrieb Daniel Herzig:
>>> Fiona Ebner <f.ebner@proxmox.com> writes:
>>>
>>>> Am 31.01.25 um 10:36 schrieb Fiona Ebner:
>>>>> Am 30.01.25 um 12:31 schrieb Daniel Herzig:
>>>>>> +
>>>>>> +    $drive->{essential} = 1 if !defined($drive->{essential});
>>>>>
>>>>> This should rather be done when parsing the drive.
>>>>
>>>> Or I guess to avoid (potentially) writing it out for every drive, it
>>>> should only be a local variable here and not set in the drive hash.
>>>
>>> Thanks, I'll double check on this for v4. But I'd assume the hash to be scoped to
>>> `config_to_command` here, or am I missing something?
>>>
>>
>> But you don't need the modified version in config_to_command() later,
>> or? And if yes, that can just check the same way again, i.e. using
>> default value if not set. If we want to explicitly set the value, that
>> should happen during parsing the drive string. Most of the time it's
>> surprising to implicitly modify a passed-in value. Better to either
>> avoid that or if really necessary, explicitly mention it in the function
>> description.
> 
> No, I do not need the modified version of the VM-config later.

Yes, but you manipulate the drive hash, which would then get written out
modified. Maybe you are lucky and there is no writer, but I'm telling
you how to avoid that by considering the default on access. E.g. see how
this is done for the 'bios' option, we do not modify the setting but
apply the default when accessing it.

> 
> What I'm trying to achieve is a more 'forgiving' behaviour in the case
> of accidently server-side-deleted iso file/unavailable server (for whatever reason)
> attached to a VM. So this is actually aiming at `qm start`, which
> implicitly calls `config_to_command` -- without modifying the existing
> VM config at all.
> 
> If the parameter 'essential' is set to '0', `config_to_command` would --
> in case of file unavailability of the iso file -- generate a kvm startup
> command that contains "-drive 'if=none,media=cdrom,[...]" instead of
> "-drive 'file=$SOME_PATH_TO_ISO,[..]' when we at this point already know
> that $SOME_PATH_TO_ISO is unavailable/non-existent.

Yes, I understand that.

> 
> The VM-config itself is not changed, as an eg nfs-server might come back
> at a point in time later and the user might want to do something with the iso
> stored there.

This is problematic for live migration, see my initial reply. Either we
need to drop the CD from the config or we need another option "missing"
that gets set if it is missing during start-up (and cleared if it is
there during startup). Then the target of migration can also know about
it when starting its own instance of the VM. We could also do this via a
special section in the configuration, see the following series[0] since
this would be internal-only information.

> 
> If the parameter 'essential' is unset, or set to '1', the die would
> happen before `qm start` leads to an invocation of kvm, as it cannot be
> expected to lead to a successful action, if $SOME_PATH_TO_ISO is not
> reachable. This would be the exact behaviour as we have it now, just not
> letting kvm run into this situation, but detect and exit earlier, while
> `config_to_commands` iterates over all volumes via `foreach_volume`
> anyway before producing the 'final' kvm command.
> 

Right, you also modify the config itself. But you cannot rely on nobody
else later writing the config. Just because it doesn't happen right now
in the tested code paths, it doesn't mean it never will. (There can be
safe-guards [1] if we really want to ensure this). But I'd either simply
have the function return which devices should not be added later, or
explicitly write out the modified config.

Because doing config modifications implicitly is error prone and future
changes can easily activate bugs lurking in that implicit behavior.

[0]:
https://lore.proxmox.com/pve-devel/20250127112923.31703-10-f.ebner@proxmox.com/
[1]:
https://lore.proxmox.com/pve-devel/20250124105351.43428-3-f.ebner@proxmox.com/


_______________________________________________
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 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos

[Fiona Ebner]

Am 03.02.25 um 14:09 schrieb Fiona Ebner:
> This is problematic for live migration, see my initial reply. Either we
> need to drop the CD from the config or we need another option "missing"
> that gets set if it is missing during start-up (and cleared if it is
> there during startup). Then the target of migration can also know about

To clarify, here I mean "cleared if the CD is there during a startup
that is not for an incoming migration".

> it when starting its own instance of the VM. We could also do this via a
> special section in the configuration, see the following series[0] since
> this would be internal-only information.



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

[pve-devel] [PATCH qemu-server v3 3/6] fix #4225: qemuserver, test: put eject_nonrequired_isos in place

[Daniel Herzig]

This commit puts `eject_nonrequired_isos` into the foreach_volume
section of `config_to_command`.

To ensure successful package-building it also does a few adaptions to
the test framework. Namely these are:

* Mock cifs-store to appear as online against a call from
  PVE::Storage::activate_storage.
* Add an nfs-offline storage to allow for comparitative testing.
* Mock all files tested by `file_exists` as existing, except the path
  contains the 'magic string' 'I_DO_NOT_EXIST'.
* Put `log_warn` output into actual warn context to allow catching the
  warnings during testing.
* Add testcases for new behaviour triggered by `eject_nonrequired_isos`.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 PVE/QemuServer.pm                             |  2 +
 test/cfg2cmd/ide-required-iso-missing.conf    | 12 ++++++
 .../cfg2cmd/ide-required-iso-missing.conf.cmd |  0
 .../cfg2cmd/ide-required-iso-offline-nfs.conf | 12 ++++++
 .../ide-required-iso-offline-nfs.conf.cmd     |  0
 test/cfg2cmd/ide-required.conf                | 14 +++++++
 test/cfg2cmd/ide-required.conf.cmd            | 39 +++++++++++++++++++
 test/cfg2cmd/ide-unrequired-iso-missing.conf  | 12 ++++++
 .../ide-unrequired-iso-missing.conf.cmd       | 33 ++++++++++++++++
 .../ide-unrequired-iso-offline-nfs.conf       | 12 ++++++
 .../ide-unrequired-iso-offline-nfs.conf.cmd   | 33 ++++++++++++++++
 test/run_config2command_tests.pl              | 36 +++++++++++++++++
 12 files changed, 205 insertions(+)
 create mode 100644 test/cfg2cmd/ide-required-iso-missing.conf
 create mode 100644 test/cfg2cmd/ide-required-iso-missing.conf.cmd
 create mode 100644 test/cfg2cmd/ide-required-iso-offline-nfs.conf
 create mode 100644 test/cfg2cmd/ide-required-iso-offline-nfs.conf.cmd
 create mode 100644 test/cfg2cmd/ide-required.conf
 create mode 100644 test/cfg2cmd/ide-required.conf.cmd
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-missing.conf
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-missing.conf.cmd
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf.cmd

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index d151c322..b8d81a21 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3826,6 +3826,8 @@ sub config_to_command {
     PVE::QemuConfig->foreach_volume($conf, sub {
 	my ($ds, $drive) = @_;
 
+	eject_nonrequired_isos($ds, $drive, $vmid, $storecfg, $conf);
+
 	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
 	    check_volume_storage_type($storecfg, $drive->{file});
 	    push @$vollist, $drive->{file};
diff --git a/test/cfg2cmd/ide-required-iso-missing.conf b/test/cfg2cmd/ide-required-iso-missing.conf
new file mode 100644
index 00000000..7f9abf87
--- /dev/null
+++ b/test/cfg2cmd/ide-required-iso-missing.conf
@@ -0,0 +1,12 @@
+# TEST: Config with default machine type, Linux & one IDE CD-ROM with online storage and missing required ISO file.
+# EXPECT_ERROR: 'ide0: cifs-store:iso/I_DO_NOT_EXIST.iso': storage unavailable or file does not exist
+bootdisk: scsi0
+cores: 2
+ide0: cifs-store:iso/I_DO_NOT_EXIST.iso,media=cdrom,size=112M
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsi0: local:100/vm-100-disk-2.qcow2,size=10G
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/ide-required-iso-missing.conf.cmd b/test/cfg2cmd/ide-required-iso-missing.conf.cmd
new file mode 100644
index 00000000..e69de29b
diff --git a/test/cfg2cmd/ide-required-iso-offline-nfs.conf b/test/cfg2cmd/ide-required-iso-offline-nfs.conf
new file mode 100644
index 00000000..a13831bf
--- /dev/null
+++ b/test/cfg2cmd/ide-required-iso-offline-nfs.conf
@@ -0,0 +1,12 @@
+# TEST: Config with default machine type, Linux & one IDE CD-ROM with required ISO file on offline storage.
+# EXPECT_ERROR: 'ide0: nfs-offline:iso/any.iso': storage unavailable or file does not exist
+bootdisk: scsi0
+cores: 2
+ide0: nfs-offline:iso/any.iso,media=cdrom,size=112M
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsi0: local:100/vm-100-disk-2.qcow2,size=10G
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/ide-required-iso-offline-nfs.conf.cmd b/test/cfg2cmd/ide-required-iso-offline-nfs.conf.cmd
new file mode 100644
index 00000000..e69de29b
diff --git a/test/cfg2cmd/ide-required.conf b/test/cfg2cmd/ide-required.conf
new file mode 100644
index 00000000..54b3f8ee
--- /dev/null
+++ b/test/cfg2cmd/ide-required.conf
@@ -0,0 +1,14 @@
+# TEST: Config with default machine type, Linux & four IDE CD-ROMs marked as required
+bootdisk: scsi0
+cores: 2
+ide0: cifs-store:iso/zero.iso,media=cdrom,size=112M,essential=1
+ide1: cifs-store:iso/one.iso,media=cdrom,size=112M,essential=1
+ide2: cifs-store:iso/two.iso,media=cdrom,size=112M,essential=1
+ide3: cifs-store:iso/three.iso,media=cdrom,size=112M,essential=1
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsi0: local:100/vm-100-disk-2.qcow2,size=10G
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/ide-required.conf.cmd b/test/cfg2cmd/ide-required.conf.cmd
new file mode 100644
index 00000000..33c6aadc
--- /dev/null
+++ b/test/cfg2cmd/ide-required.conf.cmd
@@ -0,0 +1,39 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
+  -smp '2,sockets=1,cores=2,maxcpus=2' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -drive 'file=/mnt/pve/cifs-store/template/iso/zero.iso,if=none,id=drive-ide0,media=cdrom,format=raw,aio=threads' \
+  -device 'ide-cd,bus=ide.0,unit=0,drive=drive-ide0,id=ide0,bootindex=200' \
+  -drive 'file=/mnt/pve/cifs-store/template/iso/one.iso,if=none,id=drive-ide1,media=cdrom,format=raw,aio=threads' \
+  -device 'ide-cd,bus=ide.0,unit=1,drive=drive-ide1,id=ide1,bootindex=201' \
+  -drive 'file=/mnt/pve/cifs-store/template/iso/two.iso,if=none,id=drive-ide2,media=cdrom,format=raw,aio=threads' \
+  -device 'ide-cd,bus=ide.1,unit=0,drive=drive-ide2,id=ide2,bootindex=202' \
+  -drive 'file=/mnt/pve/cifs-store/template/iso/three.iso,if=none,id=drive-ide3,media=cdrom,format=raw,aio=threads' \
+  -device 'ide-cd,bus=ide.1,unit=1,drive=drive-ide3,id=ide3,bootindex=203' \
+  -device 'virtio-scsi-pci,id=scsihw0,bus=pci.0,addr=0x5' \
+  -drive 'file=/var/lib/vz/images/100/vm-100-disk-2.qcow2,if=none,id=drive-scsi0,format=qcow2,cache=none,aio=io_uring,detect-zeroes=on' \
+  -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,rx_queue_size=1024,tx_queue_size=256,bootindex=300' \
+  -machine 'type=pc+pve0'
diff --git a/test/cfg2cmd/ide-unrequired-iso-missing.conf b/test/cfg2cmd/ide-unrequired-iso-missing.conf
new file mode 100644
index 00000000..0bff998e
--- /dev/null
+++ b/test/cfg2cmd/ide-unrequired-iso-missing.conf
@@ -0,0 +1,12 @@
+# TEST: Config with default machine type, Linux & and one IDE CD-ROM with online storage and missing unrequired ISO file.
+# EXPECT_WARN: eject 'ide0: cifs-store:iso/I_DO_NOT_EXIST.iso': storage unavailable or file does not exist
+bootdisk: scsi0
+cores: 2
+ide0: cifs-store:iso/I_DO_NOT_EXIST.iso,media=cdrom,size=112M,essential=0
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsi0: local:100/vm-100-disk-2.qcow2,size=10G
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/ide-unrequired-iso-missing.conf.cmd b/test/cfg2cmd/ide-unrequired-iso-missing.conf.cmd
new file mode 100644
index 00000000..e6efd8a9
--- /dev/null
+++ b/test/cfg2cmd/ide-unrequired-iso-missing.conf.cmd
@@ -0,0 +1,33 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
+  -smp '2,sockets=1,cores=2,maxcpus=2' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -drive 'if=none,id=drive-ide0,media=cdrom,aio=io_uring' \
+  -device 'ide-cd,bus=ide.0,unit=0,drive=drive-ide0,id=ide0,bootindex=200' \
+  -device 'virtio-scsi-pci,id=scsihw0,bus=pci.0,addr=0x5' \
+  -drive 'file=/var/lib/vz/images/100/vm-100-disk-2.qcow2,if=none,id=drive-scsi0,format=qcow2,cache=none,aio=io_uring,detect-zeroes=on' \
+  -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,rx_queue_size=1024,tx_queue_size=256,bootindex=300' \
+  -machine 'type=pc+pve0'
diff --git a/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf b/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf
new file mode 100644
index 00000000..9a87e8a5
--- /dev/null
+++ b/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf
@@ -0,0 +1,12 @@
+# TEST: Config with default machine type, Linux & and one IDE CD-ROM with unrequired ISO file on offline storage.
+# EXPECT_WARN: eject 'ide0: nfs-offline:iso/any.iso': storage unavailable or file does not exist
+bootdisk: scsi0
+cores: 2
+ide0: nfs-offline:iso/any.iso,media=cdrom,size=112M,essential=0
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsi0: local:100/vm-100-disk-2.qcow2,size=10G
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf.cmd b/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf.cmd
new file mode 100644
index 00000000..e6efd8a9
--- /dev/null
+++ b/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf.cmd
@@ -0,0 +1,33 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
+  -smp '2,sockets=1,cores=2,maxcpus=2' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -drive 'if=none,id=drive-ide0,media=cdrom,aio=io_uring' \
+  -device 'ide-cd,bus=ide.0,unit=0,drive=drive-ide0,id=ide0,bootindex=200' \
+  -device 'virtio-scsi-pci,id=scsihw0,bus=pci.0,addr=0x5' \
+  -drive 'file=/var/lib/vz/images/100/vm-100-disk-2.qcow2,if=none,id=drive-scsi0,format=qcow2,cache=none,aio=io_uring,detect-zeroes=on' \
+  -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,rx_queue_size=1024,tx_queue_size=256,bootindex=300' \
+  -machine 'type=pc+pve0'
diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index 2feebd4a..170f20c6 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -50,6 +50,15 @@ my $base_env = {
 		    iso => 1,
 		},
 	    },
+	    'nfs-offline' => {
+		type => 'nfs',
+		export => '/srv/nfs/isos',
+		path => '/mnt/pve/nfs-offline',
+		server => '127.0.0.42',
+		content => {
+		    iso => 1,
+		},
+	    },
 	    'rbd-store' => {
 		monhost => '127.0.0.42,127.0.0.21,::1',
 		fsid => 'fc4181a6-56eb-4f68-b452-8ba1f381ca2a',
@@ -201,6 +210,14 @@ $qemu_server_module->mock(
     cleanup_pci_devices => {
 	# do nothing
     },
+    file_exists => sub {
+	my $file_path = shift;
+	return 1 if !($file_path =~ m|I_DO_NOT_EXIST|);
+    },
+    log_warn => sub {
+	my $logwarn = shift;
+	return warn("${logwarn}\n");
+    },
 );
 
 my $qemu_server_config;
@@ -393,6 +410,25 @@ $pci_module->mock(
     }
 );
 
+my $pve_storage_plugin_module = Test::MockModule->new("PVE::Storage::Plugin");
+$pve_storage_plugin_module->mock(
+    activate_storage => sub {
+       return 1;
+    },
+);
+
+my $pve_storage_cifsplugin_module = Test::MockModule->new("PVE::Storage::CIFSPlugin");
+$pve_storage_cifsplugin_module->mock(
+    check_connection => sub {
+	return 1;
+    },
+    cifs_is_mounted => sub {
+	my ($scfg, $mountdata) = @_;
+	my ($mountpoint, $server, $share) = $scfg->@{'path', 'server', 'share'};
+	return $mountpoint;
+    },
+);
+
 sub diff($$) {
     my ($a, $b) = @_;
     return if $a eq $b;
-- 
2.39.5


_______________________________________________
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 3/6] fix #4225: qemuserver, test: put eject_nonrequired_isos in place

[Fiona Ebner]

Am 30.01.25 um 12:31 schrieb Daniel Herzig:
> @@ -393,6 +410,25 @@ $pci_module->mock(
>      }
>  );
>  
> +my $pve_storage_plugin_module = Test::MockModule->new("PVE::Storage::Plugin");
> +$pve_storage_plugin_module->mock(
> +    activate_storage => sub {
> +       return 1;
> +    },
> +);
> +
> +my $pve_storage_cifsplugin_module = Test::MockModule->new("PVE::Storage::CIFSPlugin");
> +$pve_storage_cifsplugin_module->mock(
> +    check_connection => sub {
> +	return 1;
> +    },
> +    cifs_is_mounted => sub {
> +	my ($scfg, $mountdata) = @_;
> +	my ($mountpoint, $server, $share) = $scfg->@{'path', 'server', 'share'};
> +	return $mountpoint;
> +    },
> +);
> +

Can't we avoid mocking PVE::Storage::Plugin and
PVE::Storage::CIFSPlugin? It should be enough to just mock the top-level
storage API from PVE::Storage. Ideally, the qemu-server package should
not have any idea how the storage package works internally. You can add
conditionals for the storage IDs if you need specific behavior for
certain storages.

>  sub diff($$) {
>      my ($a, $b) = @_;
>      return if $a eq $b;



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

[pve-devel] [PATCH pve-manager v3 4/6] fix #4225: ui: form: isoselector: add checkbox for 'essential' param

[Daniel Herzig]

Add a checkbox for marking an iso file as required for booting via the
'essential' drive parameter.

This option is used in the backend to determine if the VM should start
up in case the configured ISO file is not available.

By default this box is not visible and disabled.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 www/manager6/form/IsoSelector.js | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/www/manager6/form/IsoSelector.js b/www/manager6/form/IsoSelector.js
index 66229e88..425b1792 100644
--- a/www/manager6/form/IsoSelector.js
+++ b/www/manager6/form/IsoSelector.js
@@ -15,12 +15,14 @@ Ext.define('PVE.form.IsoSelector', {
     insideWizard: false,
     labelWidth: undefined,
     labelAlign: 'right',
+    showRequired: false,
 
     cbindData: function() {
 	let me = this;
 	return {
 	    nodename: me.nodename,
 	    insideWizard: me.insideWizard,
+	    showRequired: me.showRequired,
 	};
     },
 
@@ -113,5 +115,25 @@ Ext.define('PVE.form.IsoSelector', {
 		},
 	    },
 	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    fieldLabel: gettext('Essential'),
+	    name: 'essential',
+	    reference: 'requiredForBoot',
+	    value: 1,
+	    cbind: {
+		nodename: '{nodename}',
+		disabled: '{!showRequired}',
+		hidden: '{!showRequired}',
+		labelWidth: '{labelWidth}',
+		labelAlign: '{labelAlign}',
+	    },
+	    allowBlank: false,
+	    listeners: {
+		change: function() {
+		    this.up('pveIsoSelector').checkChange();
+		},
+	    },
+	},
     ],
 });
-- 
2.39.5


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

[pve-devel] [PATCH pve-manager v3 5/6] fix #4225: ui: qemu: cdedit: enable 'Essential' checkbox for isos

[Daniel Herzig]

Enables the 'Essential' checkbox for the IsoSelector.

If the parameter `essential` is not set in the configuration,
it will be assumed to be true, to avoid breaking changes.

The box is checked by default for the same reason.

If unchecked, `essential=0` is written to the configuration and the VM
will attempt to start, even if the file or underlying storage is not
available (by temporarily setting the value to 'none,media=cdrom').

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 www/manager6/qemu/CDEdit.js | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/www/manager6/qemu/CDEdit.js b/www/manager6/qemu/CDEdit.js
index 3cc16205..b115debc 100644
--- a/www/manager6/qemu/CDEdit.js
+++ b/www/manager6/qemu/CDEdit.js
@@ -12,6 +12,7 @@ Ext.define('PVE.qemu.CDInputPanel', {
 	me.drive.media = 'cdrom';
 	if (values.mediaType === 'iso') {
 	    me.drive.file = values.cdimage;
+	    me.drive.essential = values.essential ? undefined : '0';
 	} else if (values.mediaType === 'cdrom') {
 	    me.drive.file = 'cdrom';
 	} else {
@@ -44,6 +45,11 @@ Ext.define('PVE.qemu.CDInputPanel', {
 	} else {
 	    values.mediaType = 'iso';
 	    values.cdimage = drive.file;
+	    if (drive.essential === '1' || drive.essential === undefined) {
+		values.essential = '1';
+	    } else {
+		values.essential = '0';
+	    }
 	}
 
 	me.drive = drive;
@@ -88,6 +94,7 @@ Ext.define('PVE.qemu.CDInputPanel', {
 			cdImageField.validate();
 		    } else {
 			cdImageField.reset();
+			delete me.drive.essential;
 		    }
 		},
 	    },
@@ -98,6 +105,7 @@ Ext.define('PVE.qemu.CDInputPanel', {
 	    nodename: me.nodename,
 	    insideWizard: me.insideWizard,
 	    name: 'cdimage',
+	    showRequired: true,
 	});
 
 	items.push(me.isosel);
-- 
2.39.5


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

[pve-devel] [PATCH pve-manager v3 6/6] fix #4225: ui: qemu: hardware: add eject button for cdroms

[Daniel Herzig]

This commit adds a button in the hardware view for quickly ejecting
unnecessary cdroms or ISO files by setting file to 'none'.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 42 +++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index c6d193fc..d8a9e187 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -541,6 +541,44 @@ Ext.define('PVE.qemu.HardwareView', {
 	    apiurl: '/api2/extjs/' + baseurl,
 	});
 
+	let eject_btn = new Proxmox.button.Button({
+	    text: gettext('Eject'),
+	    disabled: true,
+	    selModel: sm,
+	    RESTMethod: 'POST',
+	    confirmMsg: function(rec) {
+		let confirmMessage = gettext("Are you sure you want to eject '{0}' ?");
+		let isoFile = PVE.Parser.parsePropertyString(rec.data.value, 'volid').volid;
+		return Ext.htmlEncode(Ext.String.format(confirmMessage, isoFile));
+	    },
+	    handler: function(btn, e, rec) {
+		let params = {};
+		params[rec.data.key] = 'none,media=cdrom';
+		if (btn.RESTMethod === 'POST') {
+		    params.background_delay = 5;
+		}
+		Proxmox.Utils.API2Request({
+		    url: '/api2/extjs/' + baseurl,
+		    waitMsgTarget: me,
+		    method: btn.RESTMethod,
+		    params: params,
+		    callback: () => me.reload(),
+		    failure: response => Ext.Msg.alert('Error', response.htmlStatus),
+		    success: function(response, options) {
+			if (btn.RESTMethod === 'POST' && response.result.data !== null) {
+			    Ext.create('Proxmox.window.TaskProgress', {
+				autoShow: true,
+				upid: response.result.data,
+				listeners: {
+				    destroy: () => me.reload(),
+				},
+			    });
+			}
+		    },
+		});
+	    },
+	});
+
 	let efidisk_menuitem = Ext.create('Ext.menu.Item', {
 	    text: gettext('EFI Disk'),
 	    iconCls: 'fa fa-fw fa-hdd-o black',
@@ -611,6 +649,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		edit_btn.disable();
 		diskaction_btn.disable();
 		revert_btn.disable();
+		eject_btn.disable();
 		return;
 	    }
 	    const { key, value } = rec.data;
@@ -622,6 +661,7 @@ Ext.define('PVE.qemu.HardwareView', {
 
 	    const isCloudInit = isCloudInitKey(value);
 	    const isCDRom = value && !!value.toString().match(/media=cdrom/);
+	    const isEjectedCDRom = value && !!value.toString().match(/none,media=cdrom/);
 
 	    const isUnusedDisk = key.match(/^unused\d+/);
 	    const isUsedDisk = !isUnusedDisk && row.isOnStorageBus && !isCDRom;
@@ -651,6 +691,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	    resize_menuitem.setDisabled(pending || !isUsedDisk);
 
 	    revert_btn.setDisabled(!pending);
+	    eject_btn.setDisabled(!cdromCap || !isCDRom || isCloudInit || isEjectedCDRom);
 	};
 
 	let editorFactory = (classPath, extraOptions) => {
@@ -754,6 +795,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		remove_btn,
 		edit_btn,
 		diskaction_btn,
+		eject_btn,
 		revert_btn,
 	    ],
 	    rows: rows,
-- 
2.39.5


_______________________________________________
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 0/6] bugzilla #4225 -- improve handling of unavailable ISOs

[Fiona Ebner]

Am 30.01.25 um 12:31 schrieb Daniel Herzig:
> This patch series addresses bugzilla entry #4225.
> 
> Currently VMs refuse to to start if a configured isofile becomes unavailable,
> be it a deleted file or an unavailable network storage.
> 
> This patch series introduces a new parameter in Drive.pm, called 'essential'.
> Depending on whether this parameter is set or not, the situation will be handled
> differently.
> 
> If the parameter is set to 0, the configuration will temporarily changed to use
> 'none' as file for the cd drive, which allows qemu to start up the machine.
> The configuration is not changed in this process to avoid unexpected behaviour.
> Instead a log_warn will be issued.
> 
> For transition reasons an unset parameter acts like 'required=1'. In this case
> the startup process will die earlier than currently, if the file is missing or
> the underlying storage not available.

Since you use the word "transition", is there a plan to change the
default behavior at some point? IMHO that requires more rationale
(especially in the backend).

> 
> If a new VM is created from the WebGUI, a corresponding added checkbox
> is checked by default, but allows for convenient unchecking during setup time,
> eg for media that is only needed for installation time.
> 
> This patch series adds an 'Eject' button to the hardwareview in the WebGUI,
> which can be used as a convenience shortcut to setting file to 'none' for the
> cdrom drive.
> 
> This series supersedes:
> https://lore.proxmox.com/pve-devel/20250113085608.99498-1-d.herzig@proxmox.com/


_______________________________________________
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 0/6] bugzilla #4225 -- improve handling of unavailable ISOs

[Daniel Herzig]

Fiona Ebner <f.ebner@proxmox.com> writes:

> Am 30.01.25 um 12:31 schrieb Daniel Herzig:
>> This patch series addresses bugzilla entry #4225.
>> 
>> Currently VMs refuse to to start if a configured isofile becomes unavailable,
>> be it a deleted file or an unavailable network storage.
>> 
>> This patch series introduces a new parameter in Drive.pm, called 'essential'.
>> Depending on whether this parameter is set or not, the situation will be handled
>> differently.
>> 
>> If the parameter is set to 0, the configuration will temporarily changed to use
>> 'none' as file for the cd drive, which allows qemu to start up the machine.
>> The configuration is not changed in this process to avoid unexpected behaviour.
>> Instead a log_warn will be issued.
>> 
>> For transition reasons an unset parameter acts like 'required=1'. In this case
>> the startup process will die earlier than currently, if the file is missing or
>> the underlying storage not available.
>
> Since you use the word "transition", is there a plan to change the
> default behavior at some point? IMHO that requires more rationale
> (especially in the backend).
>
I just used this phrasing to underline that the option 'essential', if pro-actively
set to 0, will change current behaviour. Without intervention, people should not
notice any difference to current behaviour with the changes from these
patches. Except the startup error additionaly listing the file, that's
triggering the contact to a non-available network share.

I might have a slight preference to not see ISOs as essential files
after a successful installation indeed, but there certainly are
setups, where it does not make sense to attempt starting up a machine
(or let the attempt fail) without an available ISO attached to the
VM. I'm eg thinking about ephemeral VMs running from live-ISOs or the like.

>> 
>> If a new VM is created from the WebGUI, a corresponding added checkbox
>> is checked by default, but allows for convenient unchecking during setup time,
>> eg for media that is only needed for installation time.
>> 
>> This patch series adds an 'Eject' button to the hardwareview in the WebGUI,
>> which can be used as a convenience shortcut to setting file to 'none' for the
>> cdrom drive.
>> 
>> This series supersedes:
>> https://lore.proxmox.com/pve-devel/20250113085608.99498-1-d.herzig@proxmox.com/


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