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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 59BEEE4C4 for ; Fri, 9 Dec 2022 11:30:51 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1C4FC228EA for ; Fri, 9 Dec 2022 11:30:51 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 9 Dec 2022 11:30:46 +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 AE0314426B for ; Fri, 9 Dec 2022 11:30:46 +0100 (CET) From: Fiona Ebner To: pve-devel@lists.proxmox.com Date: Fri, 9 Dec 2022 11:30:41 +0100 Message-Id: <20221209103041.58273-1-f.ebner@proxmox.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 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 Subject: [pve-devel] [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: Fri, 09 Dec 2022 10:30:51 -0000 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(-) diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm index dc6e79d..9047504 100644 --- a/PVE/Storage/RBDPlugin.pm +++ b/PVE/Storage/RBDPlugin.pm @@ -767,8 +767,6 @@ sub volume_snapshot_rollback { sub volume_snapshot_delete { my ($class, $scfg, $storeid, $volname, $snap, $running) = @_; - return 1 if $running && !$scfg->{krbd}; # FIXME: ???? - $class->deactivate_volume($storeid, $scfg, $volname, $snap, {}); my ($vtype, $name, $vmid) = $class->parse_volname($volname); -- 2.30.2