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/17/25 12:34, Daniel Herzig wrote:
> Thanks for this review, this helps a lot on setting up an optimized v3.
> 
> Daniel Kral <d.kral@proxmox.com> writes:
> 
>>
>>> 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?
> 
> I have no hard feelings about the naming of this parameter. Indeed,
> earlier it already had other names as well already.
> 
> I think the only reason why this obviously best-matching label did not
> come into closer consideration, is that on parameter definition this
> would lead to the possibly confusing construction:
> 
> # optional => {
> #     [..]
> #     optional => 1,
> #     [...]
> 
> I'm not entirely sure, if this could lead to unexpected side-effects
> despite looking funny.
> 
> I'm open for different parameter-naming though!

Good question, I'm not entirely sure about that either, but AFAIK the 
boolean `optional` property for parameters should be parsed differently 
than the hash `optional` parameter definition by JSONSchema... We do 
something similar with the parameter `type` that is both used to define 
the parameter's type (e.g. string, array) or as a parameter itself (e.g. 
storage type, VM machine type), but there could be edge cases for this 
as we don't use a parameter named `optional` anywhere else.

On second thought, I think there sure is a better name for this that 
describes what it's doing more discretely, but I'm not entirely sure 
what. Something I just came up with is 
"eject-when-missing"/"ignore-when-missing", but it is a little bit too 
long IMO and also the first one fixates itself to be only used for CDROM 
ISOs even when there would be a place for a similar parameter for other 
media types in the future.

But that's just my two cents and I might just overthink this, maybe 
someone else has a better opinion on this?

> 
>>
>>> 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".
>>
> 
> With the patches applied, this will be handled that way when a new VM gets
> created through the GUI (the box is unchecked by default in this case).
> So currently it's kind of soft-defaulting to 'required=0' with visual feedback.
> 
> But I'm not against rather propagating 'required=1' for 8.x.
> 
> To avoid conflicts with automated setup via 'qm create' that possibly
> depend on attached ISOs after intial installation nothing will be
> set at all on 'headless' actions.

Good thinking!

I'm also not sure how we handle "API stability" for the WebGUI as we're 
more often concerned about the actual API. I'm just thinking about users 
that have clicked through the "Create VM" dialog thousands of times, 
which might not catch that CDROM images are optional by default now, but 
they are the ones who would've catched this at the first VM start.

> 
>>> 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 :).
>>
> 
> True :).
> 
>>> This series supersedes:
>>> https://lore.proxmox.com/pve-devel/20241025150243.134466-1-d.herzig@proxmox.com/
>>
>> 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.
> 
> Thanks, good catch, they'll be back in v3!

Looking forward to it! :)


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