From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
	by lore.proxmox.com (Postfix) with ESMTPS id 126661FF161
	for <inbox@lore.proxmox.com>; 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 <d.kral@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>

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 <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 <d.kral@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel