From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 522A19459E for ; Wed, 11 Jan 2023 16:44:55 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 28376207C0 for ; Wed, 11 Jan 2023 16:44:25 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 11 Jan 2023 16:44:24 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 39D38402D1 for ; Wed, 11 Jan 2023 16:44:24 +0100 (CET) Message-ID: <0122c184-5dce-c8f5-9f66-455356c49616@proxmox.com> Date: Wed, 11 Jan 2023 16:44:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:109.0) Gecko/20100101 Thunderbird/109.0 Content-Language: en-GB To: Proxmox VE development discussion , Fiona Ebner References: <20221209103041.58273-1-f.ebner@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20221209103041.58273-1-f.ebner@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.038 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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. [rbdplugin.pm] Subject: [pve-devel] applied: [PATCH storage] fix #4390: rbd: snapshot delete: avoid early return to fix handling TPM drive 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: , X-List-Received-Date: Wed, 11 Jan 2023 15:44:55 -0000 Am 09/12/2022 um 11:30 schrieb Fiona Ebner: > The only caller where $running can even be truthy is QemuServer.pm's > qemu_volume_snapshot_delete(). But there, a check if snapshots should > be done with QEMU is already made and the storage function is only > called if snapshots should not be done with QEMU (like for TPM drives > which are not attached directly). So rely on the caller and do not > return early to fix removing snapshots in such cases. > > Even if a stray call ends up here (can happen already by changing the > krbd setting while a VM is running to flip the above-mentioned check > and the early return check removed by this patch), it might not even > be problematic. At least a quick test worked fine: > 1. take snapshot via a monitor command in QEMU > 2. remove snapshot via the storage layer > 3. create a new file in the VM > 4. take a snapshot with the same name via monitor command in QEMU > 5. roll back to the snapshot > 6. check that the file in the VM is as expected > Using the storage layer to take the snapshots and QEMU to remove the > snapshot also worked doing the same test. Even if it were problematic, > the check in qemu-server should rather be improved then. > > (Trying to issue a snapshot mon command for a krbd-mapped image fails > with an error on the other hand, but that is also not too bad and not > relevant to the storage code. Again, it rather would be necessary to > improve the check in qemu-server). > > The fact that the pve-container code didn't even pass $running is the > reason removing snapshots worked for containers on a storage with krbd > disabled (the pve-container code calls map_volume() explicitly, so > containers can work regardless of the krbd setting in the storage > configuration; see commit 841fba6 ("call map_volume before using > volumes.") in pve-container). > > For volume_snapshot(), the early return with $running was already > removed (or rather the relevant logic moved to QemuServer.pm) in 2015 > by commit f5640e7 ("remove running from Storage and check it in > QemuServer"), even before krbd support was added in RBDPlugin.pm. > > Signed-off-by: Fiona Ebner > --- > PVE/Storage/RBDPlugin.pm | 2 -- > 1 file changed, 2 deletions(-) > > applied, with Fabian's R-b, thanks! Can you please try to keep track of removing the $running param altogether with the future Proxmox VE 8.x?