From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <pve-devel-bounces@lists.proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 62F271FF16F for <inbox@lore.proxmox.com>; Thu, 16 Jan 2025 16:18:41 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AA92E2C66A; Thu, 16 Jan 2025 16:18:37 +0100 (CET) Message-ID: <6fc390ea-d36a-4b0a-a138-2b5aa47624c3@proxmox.com> Date: Thu, 16 Jan 2025 16:18:33 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>, Daniel Herzig <d.herzig@proxmox.com> References: <20250113085608.99498-1-d.herzig@proxmox.com> Content-Language: en-US From: Daniel Kral <d.kral@proxmox.com> In-Reply-To: <20250113085608.99498-1-d.herzig@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.011 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 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 <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com> Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" <pve-devel-bounces@lists.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