From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id E14B81FF16B for ; Tue, 15 Jul 2025 16:49:33 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8ECC39692; Tue, 15 Jul 2025 16:50:30 +0200 (CEST) Date: Tue, 15 Jul 2025 16:49:55 +0200 From: Wolfgang Bumiller To: "DERUMIER, Alexandre" Message-ID: References: <20250709162202.2952597-1-alexandre.derumier@groupe-cyllene.com> <46hm62jbunky6a53hid6fh6honfkncoccimej7iidh52gr34hy@xumdyi4czqoi> <4756bd155509ba20a3a6bf16191f1a539ee5b23e.camel@groupe-cyllene.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4756bd155509ba20a3a6bf16191f1a539ee5b23e.camel@groupe-cyllene.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.078 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_MSPIKE_H2 0.001 Average reputation (+2) 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 pve-storage 09/13] storage: add volume_support_qemu_snapshot 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 Cc: "pve-devel@lists.proxmox.com" , "t.lamprecht@proxmox.com" Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On Tue, Jul 15, 2025 at 01:59:33PM +0000, DERUMIER, Alexandre wrote: > = > > +* Introduce volume_support_qemu_snapshot() plugin method > > +=A0=A0=A0 This method is used to known if the a snapshot need to be do= ne > > by qemu > > +=A0=A0=A0 or by the storage api. > > +=A0=A0=A0 returned values are : > > +=A0=A0=A0 'internal' : support snapshot with qemu internal snapshot > > +=A0=A0=A0 'external' : support snapshot with qemu external snapshot > > +=A0=A0=A0 undef=A0=A0=A0=A0=A0 : don't support qemu snapshot > = > >>The naming, description and returned values seem a bit of a mixed bag > >>here. > >>Also because "internal", from the POV of the storage, happens > >>"outside" of the storage. > yes, indeed, I was a little bit out of idea for the naming, sorry :/ > = > = > = > >>Also consider that technically the *beginning* of a snapshot chain > >>can > >>even have a raw format, so I also wouldn't specifically pin the > >>ability > >>to create a new volume using a backing volume on the current *v > >olume*. > = > yes, Fabian wanted to use only qcow2 for the base image for now, to > avoid to mix both format in the chain > = > = > >>As a side note: Looking at the LVM plugin, it now reports "qcow2" > >>support, but refuses it unless an option is set. The list of which > >>formats are supported is currently not storage-config dependent, > >>which > >>is a bit unfortunate there. > = > do you mean my last follow-up about "external-snapshots" option for lvm > ? Because Fabian && Thomas wanted it as safe-guard until it's still > experimental > = > = > = > >>How about one of these (a bit long but bear with me): > >>=A0 supports_snapshots_as_backed_volumes($scfg) > >>=A0=A0=A0 The storage: > >>=A0=A0=A0 - Can create volumes with backing volumes (required for NOT- > >>running > >>=A0=A0=A0=A0=A0 VMs) > >>=A0=A0=A0 - Allows renaming a volume into a snapshot such that calling > >>=A0=A0=A0=A0=A0 `volume_snapshot` then recreates the original name... (= BUT! See > >>=A0=A0=A0=A0=A0 note [1] below for why I think this should not be a > >>requirement!) > >>=A0=A0=A0=A0=A0 (for running VMs) > >>combined with > >> > >>=A0 is_format_available($scfg, "qcow2") > >> > >>(to directly check for the optional support as in the LVM plugin > early) > = > >>qemu-server's do_snapshot_type() then does: > >> > >>=A0=A0=A0 "storage" if > >>=A0=A0=A0=A0=A0=A0 is tpmstate > >>=A0=A0=A0=A0=A0=A0 OR is not running > >>=A0=A0=A0=A0=A0=A0 OR format isn't qcow2 (this case would be new in qem= u-server) > >>=A0=A0=A0 "external" if > >>=A0=A0=A0=A0=A0=A0 supports_snapshots_as_backed_volumes() is true > >>=A0=A0=A0=A0=A0=A0 AND is_format_available($scfg, "qcow2") > >>=A0=A0=A0 "internal" otherwise > = > you also have the case for rbd, where krbd need to be done at storage > level, but rbd need to be done by qemu when it's running. > = > So I think it's more something like : > -- OR format isn't qcow2 (this case would be new in qemu-server) > ++ OR qemu block driver support .bdrv_snapshot_create = > (currently it's only qcow2 && rbd : > https://github.com/search?q=3Drepo%3Aqemu%2Fqemu%20%20.bdrv_snapshot_crea= te%20&type=3Dcode) Right, I forgot about that. Okay, so I guess we'd keep this qemu-related. In that case: sub qemu_snapshot_responsibility($scfg, $volname) -> qemu (previously "internal") -> storage (previously undef) -> mixed (previously "external") We should probably note in the API documentation that "mixed" is experimental and the expected behavior may change, because, as I mentioned further down previously, I think the behavior could be a bit less awkward on the storage side. > = > = > = > = > = > >>Notes: > >>[1] Do we really need to rename the volume *outside* of the storage? > >>Why does eg. the LVM plugin need to distinguish between running and > >>not > >>running at all? All it does is shove some `/dev/` names around, after > >>all. We should be able to go through the change-file/reopen code in > >>qemu-server regardless of when/who/where renames the files, no? > >> > >>Taking a snapshot of a running VM does: > >>=A0 1. rename volume "into" snapshot from out of qemu-server > >>=A0 2. tell qemu to reopen the files under the new names > >>=A0 3. call volume_snapshot which: > >>=A0=A0=A0 - creates volume with the original name and the snapshot as > >>backing > >>=A0 4. have qemu open the disk by the original name for blockdev- > snapshot > = > = > Can't we make this: > >>=A0 1. tell qemu to reopen the original files with different *node ids* > >>=A0=A0=A0=A0 (since the clashing of those is the only issue we run into= when > >>=A0=A0=A0=A0 simply dropping the $running case and skipping the early= =A0 > >>rename...) > >>=A0 2. call volume_snapshot without a $running parameter > >>=A0 3. continue as before (point 4 above) > = > ok, so we could try to = > 1) reopen the current volume (vm-disk-..) with nodename=3Dvolname+snap > before renaming it, > 2) do the volume_snapshot in storage (rename the current volume to snap > , create a new current volume with the snap backing) > 3) add the new current volume blockdev + reopen it with blockdev- > snapshot. > = > Here, I'm not sure if it's working, because AFAIR, when you call > blockdev-snapshot,=A0 > mon_cmd( = > $vmid, 'blockdev-snapshot', > node =3D> $snap_fmt_blockdev->{'node-name'}, > overlay =3D> $new_fmt_blockdev->{'node-name'}, > ); > = > it's reading the filename in the snap blockdev node (and in this case, > it's still vm-disk-...) and write it in the backing_file info of the > new overlay node. > = > I think I have tried in past, but I'm not 100% sure. I'll try to test > it again this week. Yeah I tried some quick tests and it seems to be a bit tricky. Or maybe I just missed something. > = > = > = > I'm going on holiday for 3 weeks this Friday, so I'll not have time to > sent patch before nest, but feel free to change|improve|rewrite my code > by something better :) _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel