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