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 A2F8E917A9 for ; Wed, 21 Dec 2022 15:36:30 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8122B994B for ; Wed, 21 Dec 2022 15:36:30 +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, 21 Dec 2022 15:36:29 +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 260A944F04 for ; Wed, 21 Dec 2022 15:36:29 +0100 (CET) Date: Wed, 21 Dec 2022 15:36:21 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20221209103041.58273-1-f.ebner@proxmox.com> In-Reply-To: <20221209103041.58273-1-f.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1671632666.ygmhc5lnca.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.133 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, proxmox.com] Subject: Re: [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: Wed, 21 Dec 2022 14:36:30 -0000 On December 9, 2022 11:30 am, Fiona Ebner wrote: > 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. >=20 > 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. >=20 > (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). >=20 > 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). >=20 > 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. given the last two paragraphs, and the following: - some plugins don't even have $running as parameter in the signature list - qemu-server is not calling this code for librbd, qcow2 or qed files - that (guarded) call site in qemu-server is the only one that actually set= s $running to anything other than undef or 0 (AFAICT) - only the RBDPlugin (this patch) and the DirPlugin and its descendants (wh= ich only supports qcow2 and qed for snapshotting, which are only supported by qemu-server, and handled directly by Qemu for running VMs) are handling $running at all we should probably mark this parameter for dropping with 8.0 (both in the p= lugin interface and in PVE::Storage itself)? external plugins already cannot hook into the "use qemu for snapshotting" l= ogic (which could of course be changed, although I am not sure if there's much benefit), and only leaving this parameter in for snapshot deletion doesn't = bring any benefit. I kinda doubt that any external plugins are using this paramet= er, but technically removing it earlier could be considered a breaking change.. other than this, consider this Reviewed-by: Fabian Gr=C3=BCnbichler >=20 > Signed-off-by: Fiona Ebner > --- > PVE/Storage/RBDPlugin.pm | 2 -- > 1 file changed, 2 deletions(-) >=20 > 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) =3D @_; > =20 > - return 1 if $running && !$scfg->{krbd}; # FIXME: ???? > - > $class->deactivate_volume($storeid, $scfg, $volname, $snap, {}); > =20 > my ($vtype, $name, $vmid) =3D $class->parse_volname($volname); > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20