From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 18C171FF164 for ; Fri, 17 Jan 2025 14:05:20 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D94C6ED7F; Fri, 17 Jan 2025 14:05:15 +0100 (CET) Message-ID: <1845e66b-9a2c-47d3-8ec6-c0f24819eb17@proxmox.com> Date: Fri, 17 Jan 2025 14:04:41 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Daniel Herzig References: <20250113085608.99498-1-d.herzig@proxmox.com> <6fc390ea-d36a-4b0a-a138-2b5aa47624c3@proxmox.com> <87frlhn079.fsf@proxmox.com> Content-Language: en-US From: Daniel Kral In-Reply-To: <87frlhn079.fsf@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.010 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: Re: [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Cc: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 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