Re: [pve-devel] [PATCH v2 storage 05/10] rbd plugin: implement volume import/export

[Daniel Kral <d.kral@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