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 CD07C8E5A7 for ; Fri, 11 Nov 2022 16:18:45 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A838C58A0 for ; Fri, 11 Nov 2022 16:18:15 +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 ; Fri, 11 Nov 2022 16:18:14 +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 BB12E43A3A for ; Fri, 11 Nov 2022 16:18:13 +0100 (CET) Message-ID: Date: Fri, 11 Nov 2022 16:18:12 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Content-Language: en-US To: Proxmox VE development discussion , Mira Limbeck References: <20221020152236.371434-1-m.limbeck@proxmox.com> From: Stefan Hanreich In-Reply-To: <20221020152236.371434-1-m.limbeck@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.577 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 NICE_REPLY_A -0.001 Looks like a legit reply (A) 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. [qemuconfig.pm] Subject: Re: [pve-devel] [PATCH v2 qemu-server] fix #4201: delete cloud-init disk on rollback 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, 11 Nov 2022 15:18:45 -0000 Test Setup: I created a new VM, without any Cloud-Init drive, and immediately created a snapshot. Then I setup a Cloud-Init drive according to the PVE documentation via CLI. I created another snapshot of this state with a Cloud-Init drive. What I tested: - Rolling back to the older snapshot without Cloud-Init drive, it got successfully removed. - Rolling back to the newer snapshot that includes a Cloud-Init drive, it successfully showed up afterwards. I also tested both of those cases with Cloud-Init drives created from the Web UI, in case there are any differences. Some additional tests including normal CD-ROM drives I did as well - just in case: - Rolled back from a snapshot with only a Cloud-Init drive to a snapshot that contains only a CD-ROM drive, the CD-ROM drive showed up afterwards and removed the Cloud-Init drive successfully - Rolled back from a snapshot with only a CD-ROM drive to a state that contains only a Cloud-Init drive, the Cloud-Init drive showed up afterwards and removed the CD-ROM drive successfully Code LGTM Reviewed-by: Stefan Hanreich Tested-by: Stefan Hanreich On 10/20/22 17:22, Mira Limbeck wrote: > If the config doesn't contain the cloud-init disk anymore after the > rollback, we have to clean it up since otherwise no further disk can be > attached unless the one still existing on the storage is deleted. > > Signed-off-by: Mira Limbeck > --- > v2: > - chose the add_unused_volume way as @fiona recommended, the > implementation is a lot cleaner, but contains a cloudinit regex > > - removed the 2nd patch for reusing already existing disks when > adding a cloudinit disk > > PVE/QemuConfig.pm | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm > index 482c7ab..9fb8e9f 100644 > --- a/PVE/QemuConfig.pm > +++ b/PVE/QemuConfig.pm > @@ -498,7 +498,7 @@ sub __snapshot_rollback_get_unused { > $class->foreach_volume($conf, sub { > my ($vs, $volume) = @_; > > - return if PVE::QemuServer::drive_is_cdrom($volume); > + return if PVE::QemuServer::drive_is_cdrom($volume, 1); > > my $found = 0; > my $volid = $volume->{file}; > @@ -507,7 +507,7 @@ sub __snapshot_rollback_get_unused { > my ($ds, $drive) = @_; > > return if $found; > - return if PVE::QemuServer::drive_is_cdrom($drive); > + return if PVE::QemuServer::drive_is_cdrom($drive, 1); > > $found = 1 > if ($drive->{file} && $drive->{file} eq $volid); > @@ -519,6 +519,19 @@ sub __snapshot_rollback_get_unused { > return $unused; > } > > +sub add_unused_volume { > + my ($class, $config, $volid) = @_; > + > + if ($volid =~ m/vm-\d+-cloudinit/) { > + print "found unused cloudinit disk '$volid', removing it\n"; > + my $storecfg = PVE::Storage::config(); > + PVE::Storage::vdisk_free($storecfg, $volid); > + return undef; > + } else { > + return $class->SUPER::add_unused_volume($config, $volid); > + } > +} > + > # END implemented abstract methods from PVE::AbstractConfig > > sub has_cloudinit {