Re: [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs

[Daniel Kral <d.kral@proxmox.com>]

On 1/13/25 09:55, Daniel Herzig wrote:
> 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 'required'.
> Depending on whether this parameter is set or not, the situation will be handled
> differently.

Thanks for tackling this, this is a great addition to the CDROM drives!

I'm looking forward to this being merged as it happens quite frequently 
when setting up a lot of VMs with different ISOs!

> 
> 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.

Hm, I have discussed with Friedrich about this off-list, because I'm 
thinking about "optional" being another name for this flag, since it 
should be required by default for VMs that are not explicitly setting 
this option, i.e. `optional=0`, and if someone sets it explicitly to 
`optional=1` the CDROM can be ignored if it is non-existent.

I think this could also simplify the logic overall, but it depends on 
how we want to present this to users (i.e. the WebGUI).

Are there reasons against this? What do you think?

> 
> If however a new VM is created from the WebGUI, the corresponding added checkbox
> is not checked by default, and the resulting 'required=0' will be written to
> the configuration.

IMO, I also think that new VMs should be set to `required=0` by default, 
but this change should probably be postponed to 9.0 as it would break 
the current WebGUI "user-API".

> 
> To allow for proper testing and building some additions and minor changes
> where made to to the testing framework as well.
> 
> Not exactly part of #4225, but related to it, this patch series adds an 'Eject'
> button to the hardwareview in the WebGUI, which can be used as a convenience
> shortcut to manually editing the missing ISO file to 'Do not use any media'.

In this case it is better to move unrelated changes into a separate 
patch series, so they can be reviewed on their own :).

> 
> This series supersedes:
> https://lore.proxmox.com/pve-devel/20241025150243.134466-1-d.herzig@proxmox.com/
> 
> Changes from initial series:
> * rebased onto current master
> * fix cifs mocking in run_config2command_tests.pl
> * fix expected outcome in ide-required.conf.cmd
> 
> qemu-server: Daniel Herzig (9):
>    fix #4225: qemuserver: drive: add optional required parameter
>    qemuserver: add helper function for mocking files
>    fix #4225: qemuserver: add function to eject isofiles
>    test: chomp all trailing newlines from errors and warnings
>    test: mock cifs-store
>    test: add nfs-offline storage
>    test: mock existing files
>    test: mock log_warn warnings
>    test: cfg2cmd: add tests for testing the iso required parameter
> 
>   PVE/QemuServer.pm                             | 44 +++++++++++++++++++
>   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              | 44 ++++++++++++++++++-
>   13 files changed, 261 insertions(+), 3 deletions(-)
>   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
> 
> manager: Daniel Herzig (3):
>    fix #4225: ui: form: isoselector: add optional required checkbox
>    fix #4225: ui: qemu: cdedit: enable required checkbox for isos
>    ui: qemu: hardware: add eject button for cdroms
> 
>   www/manager6/form/IsoSelector.js  | 22 ++++++++++++++++
>   www/manager6/qemu/CDEdit.js       |  6 +++++
>   www/manager6/qemu/HardwareView.js | 43 +++++++++++++++++++++++++++++++
>   3 files changed, 71 insertions(+)
> 
> 

I also just noticed that the repository names are gone from the patches 
- seems like they were accidentally removed when formatting the second 
version of these patches because they were there in v1.

All things considered, this is a great feature addition I'd like to see 
merged, so consider the whole patch series as:

Reviewed-by: Daniel Kral <d.kral@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>



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