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 126661FF161 for ; Wed, 18 Dec 2024 15:20:07 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8376017A8F; Wed, 18 Dec 2024 15:20:06 +0100 (CET) Message-ID: <06d3843b-2e72-4394-ad79-a174195f7301@proxmox.com> Date: Wed, 18 Dec 2024 15:20:02 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: pve-devel@lists.proxmox.com References: <20241217154814.82121-1-f.ebner@proxmox.com> <20241217154814.82121-6-f.ebner@proxmox.com> Content-Language: en-US From: Daniel Kral In-Reply-To: <20241217154814.82121-6-f.ebner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.005 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_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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 v2 storage 05/10] rbd plugin: implement volume import/export 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 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On 12/17/24 16:48, Fiona Ebner wrote: > [ ... ] > + > + die "volume export format $format not available for $class\n" if $format ne 'raw+size'; > + die "cannot export volumes together with their snapshots in $class\n" if $with_snapshots; > + die "cannot export an incremental stream in $class\n" if defined($base_snapshot); > + > + my $file = $class->map_volume($storeid, $scfg, $volname, $snapshot); > + eval { > + nit: unnecessary newline > + my $size; > + # should be faster than querying RBD, also checks for the device file's availability > + run_command(['/sbin/blockdev', '--getsize64', $file], outfunc => sub { > + my ($line) = @_; > + die "unexpected output from /sbin/blockdev: $line\n" if $line !~ /^(\d+)$/; > + $size = int($1); > + }); > [ ... ] > + > + die "volume import format $format not available for $class\n" if $format ne 'raw+size'; > + die "cannot import volumes together with their snapshots in $class\n" if $with_snapshots; > + die "cannot import an incremental stream in $class\n" if defined($base_snapshot); > + > + my (undef, $name, $vmid, undef, undef, undef, $file_format) = $class->parse_volname($volname); nit: is there any downside to `my ($name, $vmid, $format) = ($class->parse_volname($volname))[1,2,6];` > + die "cannot import format $format into a volume of format $file_format\n" > + if $file_format ne 'raw'; > [ ... ] Besides the two minor remarks, the `volume_{import,export}{,_formats}` subs look good to me (wrt. the LVMPlugin implementation it is based on). In the future it would probably be nice to factor out some common logic between the RBD, LVM and (now) ISCSI storage plugin. With respect to patch #2, #4, #5, #8, #9, #10 and the followup patch #1 from [0], I've tested the manual exports, manual imports and remote migrations of VMs and container with rbd storages. - When exporting with "pvesm export ... --with-snapshots 1" I get an expected error - When exporting with any format besides "raw+size" I get an expected error - When exporting with "pvesm export ... --snapshot " the volume is correctly mapped and exported - When exporting with "pvesm export ...", the volume has the same checksum as with "rbd export ..." with the size header prepended - When importing with "--allow-rename" the volume is correctly renamed - Remote migration works for VMs with `qm remote-migrate ...` from RBD to directory storage (ext4, xfs) and lvm - Remote migration works for container with `pct remote-migrate ...` (after also applying patch #1 from the follow up) from RBD to directory storage (ext4, xfs) and lvm I checked the integrity of the volumes with `md5sum` and if they boot up fine. The only thing that I noticed - which is probably unrelated to this patch series - is that VMs as well as container have "migrate" locks after successful remote migrations (the lock is removed if I kill the migration during the process). From the log output alone it seems that the local VM/CT is never unlocked, only the one on the remote. I also tested importing the volumes with `pvesm import ...`, which I had exported before with `pvesm export ...`, which worked just as expected. Consider this patch as: Reviewed-by: Daniel Kral Tested-by: Daniel Kral _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel