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 71CFB1FF17A for ; Tue, 28 Oct 2025 13:10:28 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A7A1D19B43; Tue, 28 Oct 2025 13:10:58 +0100 (CET) Message-ID: <85dab76c-d461-43d1-8f1c-45ea1b31200a@proxmox.com> Date: Tue, 28 Oct 2025 13:10:54 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Friedrich Weber References: <20251024122705.93761-1-f.weber@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20251024122705.93761-1-f.weber@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1761653442731 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.021 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] [RFC qemu-server/storage 0/3] fix #5779: introduce guest hints to pass rxbounce flag to KRBD 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 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Am 24.10.25 um 2:27 PM schrieb Friedrich Weber: > - The biggest obstacle is that we need to update all callers of > activate_volumes to pass guest hints where possible. This means that optimally > all callers should be able to generate the guest hints. Right now, there is > only the 'guest-ostype' hint which is taken from the VM config. Currently, this > is not always available to the caller of activate_volumes, sometimes > some extra work/refactoring would be needed to get it (e.g. see > PVE::QemuServer::QemuImage::convert or PVE::QemuServer::clone_disk), > so this needs quite some code changes, which I have not done in all cases in > this RFC. See below. > - There are also some indirect callers of activate_volumes, e.g. via > PVE::Storage::abs_filesystem_path or PVE::Storage::storage_migrate -- these > would also need to be extended to accept hints (not done in this RFC) See below. > - Initially, to avoid having to modify all (direct+indirect) callers of > activate_volumes, I thought I could pass the hints only at the few "relevant" > call sites (i.e., when starting a VM), but then noticed that volumes may be > activated by an action unrelated to a VM start (e.g. a clone), then stay > active, and not be re-activated by a VM start. So if e.g. we do not pass the > hints on clone, the KRBD volume would be mapped without rxbounce, stay active, > and when starting the VM, a user could run into the original problem again. > So we can't get away with only passing hints to the few relevant call sites, > and actually need to pass them everywhere (where possible). See other mail. > - But sometimes, it might be fine to not pass hints to activate_volumes because > we immediately deactivate, e.g. in QemuServer::restore_vma_archive, but only > because the only current hint 'guest-ostype' is not relevant in such cases - > might not be the case for future hints? Ideally, if we go with the current approach, we would cover everything so that we'd only need to adapt the generate_storage_hints() function. But something completely different which comes to mind here. What about the volumes that already are active immediately after allocation? For those, we either need to: 1. deactivate them and reactivate them with hints after allocation 2. or pass along the hints during allocation 3. ensure that they are not used in situations where hints are required before deactivation. The alternative approach of "make sure to deactivate before using hints in situations where actually required" would avoid that complication too (with 1. being very similar but here it would be immediately after allocation). > - 'guest-ostype' might be the only hint we need for now and can be taken from > the guest config, but can we be sure that future hints are also going to be > derived from the guest config? Is it OK to have generate_storage_hints only > take the guest config as well? This can easily be changed later when the need arises. > - minor, but still: with this approach, rxbounce would also be passed for the > EFI disk and TPM state of Windows VMs. This is likely not needed, but probably > OK for a first version? Can be okay, but see below. > - should we already pass hints when activating volumes in pve-container too, > even though they are empty? Not strictly required IMHO, but feel free to if you like. > - is it even OK to pass the hints only to {activate,map}_volumes? What if an > external storage plugin needs the hint also when deactivating a volume? Should > we already add a $hints parameter to other plugin methods as well, to avoid > having to do that (and another APIVER bump) later? I'd prefer to add it to deactivate too. > - With hints being an argument to activate_volumes which takes a list of volumes, > the hint applies to all volumes in the list. Is this OK API-wise? For the > guest-ostype hint, this only makes sense if all passed volumes belong to the > same guest and not several different ones (because they may have different OS > types), which I think is the case for our call sites, but doesn't have to be > the case? I'd prefer it to be per-volume, so that we can leave out TPM and EFI disks and things like the $statefile when that is a volume. Can be done by the current interface by making multiple calls of course, but that is not ideal. Maybe one of the future hints will be precisely for TPM or EFI, those seem like likely candidates for requiring special treatment. We could deprecate activate_volumes() and either: 1. add a new function which takes a hash reference rather than an array reference. The keys would be volid with values being additional options, like snapshot name and hints. 2. Or we could have a mini-class PVE::Storage::Activate which would have a method to activate a single volume, but keeps track of which storages and volumes it already activated in its internal state and which could then also have a method for deactivating what it activated (like is needed after VM start failure). But please note that this is independent of the plugin API and does not need to be done at the same time as the changes there (AFAICS). _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel