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.k...@proxmox.com>
Tested-by: Daniel Kral <d.k...@proxmox.com>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
