On 1/30/24 11:00, Fabian Grünbichler wrote:
On December 19, 2023 3:03 pm, Hannes Duerr wrote:
for base images we call the volume_import of the parent plugin and pass
it as vm-image instead of base-image, then convert it back as base-image
Signed-off-by: Hannes Duerr <h.du...@proxmox.com>
---
src/PVE/Storage/LvmThinPlugin.pm | 50 ++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/src/PVE/Storage/LvmThinPlugin.pm b/src/PVE/Storage/LvmThinPlugin.pm
index 1d2e37c..96f619b 100644
--- a/src/PVE/Storage/LvmThinPlugin.pm
+++ b/src/PVE/Storage/LvmThinPlugin.pm
@@ -383,6 +383,56 @@ sub volume_has_feature {
return undef;
}
+sub volume_import {
+ my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot,
$base_snapshot, $with_snapshots, $allow_rename) = @_;
+
+ my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
+ $class->parse_volname($volname);
+
+ if (!$isBase) {
+ return $class->SUPER::volume_import(
+ $scfg,
+ $storeid,
+ $fh,
+ $volname,
+ $format,
+ $snapshot,
+ $base_snapshot,
+ $with_snapshots,
+ $allow_rename
+ );
+ } else {
+ my $tempname;
+ my $vg = $scfg->{vgname};
+ my $lvs = PVE::Storage::LVMPlugin::lvm_list_volumes($vg);
+ if ($lvs->{$vg}->{$volname}) {
+ die "volume $vg/$volname already exists\n" if !$allow_rename;
+ warn "volume $vg/$volname already exists - importing with a different
name\n";
+
+ $tempname = $class->find_free_diskname($storeid, $scfg, $vmid);
+ } else {
+ $tempname = $volname;
+ $tempname =~ s/base/vm/;
couldn't we simply do this part unconditionally, and then call the SUPER
method? that one already checks whether $volname exists, does the same
die/warn and then unsets $volname in case of a collision, so that
alloc_image gets to pick a free one? then we just need to create_base on
the returned $volid and be done, making this a very thin wrapper..
If we simply rename base-image-x to vm-image-x and pass it on to the parent
image, we may not be able to create a base image of the imported volume
vm-image-x, as there may already be a base-image-x, which we have not
checked
before.
The patch still applies on master
but, I just realized a bigger issue with all of this code here - AFAICT
nothing ensures the storage is actually locked, so two parallel
imports (or an import racing with an alloc/..) could actually end up
trying to use the same volname. but we don't want to hold the lock for
the full duration of the import, just for the part covering the "find
free slot -> alloc_image" time span. this is not limited to LVM, but
properly handling it would require splitting up volume_import into two
plugin methods (breaking the storage plugin ABI) which might not even
work for all storage types.
Created a patch for this issue:
https://lists.proxmox.com/pipermail/pve-devel/2024-April/062581.html
+ }
+
+ ($storeid,my $newname) =
PVE::Storage::parse_volume_id($class->SUPER::volume_import(
+ $scfg,
+ $storeid,
+ $fh,
+ $tempname,
+ $format,
+ $snapshot,
+ $base_snapshot,
+ $with_snapshots,
+ $allow_rename
+ ));
+
+ $volname = $class->create_base($storeid, $scfg, $newname);
+ }
+
+ return "$storeid:$volname";
+}
+
# used in LVMPlugin->volume_import
sub volume_import_write {
my ($class, $input_fh, $output_file) = @_;
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
