fix: let path() return a placeholder for extent-less zvols (#292)

PVE's content delete (pvesm free, GUI Remove) and qm destroy call path()
only for the owner/vtype before vdisk_free. Dying when the zvol had no
iSCSI extent made orphans from failed allocs or v2-era leftovers
undeletable through PVE, even though free_image already handles the
no-extent case. activate_volume and qemu_blockdev_options keep dying on
the same condition, so the placeholder can never be opened by a VM.

Adds the first Perl unit-test harness (t/, prove in CI lint job) with a
mock TrueNAS API so plugin entry points can be tested off a PVE node.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 1bbcba136b)

.github/workflows/build.yml

@@ -73,6 +73,11 @@ jobs:
             packaging/DEBIAN-multipath/postrm
           echo "Shell scripts OK"
 
+      - name: Perl unit tests
+        run: |
+          echo "==> Running prove t/..."
+          prove -v t/
+
       - name: Test postinst/postrm service restarts
         run: |
           echo "==> Running tests/test-restart-services.sh..."

README.md

@@ -595,6 +595,18 @@ A quick reference:
 
 If you see iSCSI extents in TrueNAS that are not associated with any target, they can be safely deleted from the TrueNAS UI. v2.3.0 and later automatically roll back and clean up after a failed LUN creation.
 
+### `pvesm free` / GUI Remove fails with "has no iSCSI extent"
+
+**Symptom:** removing a disk fails with `Volume 'vm-101-disk-0' has no iSCSI extent on <host>. Was it created via this plugin?`. The zvol still exists on TrueNAS, but its iSCSI extent is gone. Typical causes are a failed `alloc_image`, an extent deleted by hand, or a leftover from v2.x.
+
+**Fixed in v4.0.2** ([#292](https://github.com/TheGrandWazoo/freenas-proxmox/issues/292)). `path()` now returns a placeholder for these orphans, so Proxmox can delete them normally. The task log shows `path: <vol> has no mapped iSCSI extent — returning placeholder path`. Starting a VM on such a disk still fails at `activate_volume`, as it should.
+
+**On v4.0.1 and earlier:** first confirm that no VM config references the disk (`grep -r '<vol>' /etc/pve/nodes/*/qemu-server/`). Then call `vdisk_free` directly, which skips the `path()` lookup:
+
+```bash
+perl -MPVE::Storage -e 'PVE::Storage::vdisk_free(PVE::Storage::config(), "STORAGE:vm-101-disk-0")'
+```
+
 ### Filing a Bug Report
 
 Please use the [GitHub issue tracker](https://github.com/TheGrandWazoo/truenas-proxmox/issues) and include the [bug report template](.github/ISSUE_TEMPLATE/bug_report.md). Include relevant log lines from `syslog` (search for `FreeNAS::`).

docs/migration-troubleshooting.md

@@ -110,6 +110,8 @@ The v2.x plugin may not have removed these automatically. They are harmless but
 
 Always verify a zvol is truly unused before deleting it — check `qm config` for all VMs.
 
+**Zvols on a v3/v4 `truenas` storage** that have no extent show up in `pvesm list <storage>`. From v4.0.2 you can delete them with `pvesm free <storage>:<vol>` or the GUI's Remove button. On older versions that fails with "has no iSCSI extent"; see the README troubleshooting entry for the `vdisk_free` workaround ([#292](https://github.com/TheGrandWazoo/freenas-proxmox/issues/292)).
+
 ---
 
 ## Old v2.x storage can't be removed from Proxmox

perl5/PVE/Storage/Custom/TrueNAS.pm

@@ -397,6 +397,14 @@ sub _find_extent {
     };
 }
 
+# Placeholder path() result for a zvol with no mapped iSCSI extent (#292).
+# Never a usable device: the IQN segment is not a valid IQN, so anything that
+# tries to open it fails at connect time rather than hitting the wrong LUN.
+sub _unmapped_path {
+    my ($scfg, $volname) = @_;
+    return 'iscsi://' . _portal($scfg) . "/unmapped/$volname";
+}
+
 # Signal TrueNAS to reload the iSCSI service so new extents are visible
 sub _reload_iscsi {
     my ($scfg) = @_;
@@ -614,12 +622,19 @@ sub path {
     my ($class, $scfg, $volname, $storeid, $snapname) = @_;
     _resolve_token($storeid, $scfg);
 
-    my $ext = _find_extent($scfg, $volname);
-    die "Volume '$volname' has no iSCSI extent on $scfg->{truenas_host}. "
-      . "Was it created via this plugin?\n" unless $ext;
+    my ($vtype, undef, $ds_vmid) = $class->parse_volname($volname);
 
-    die "Volume '$volname' is not mapped to any iSCSI target.\n"
-        unless defined $ext->{target_id};
+    # A zvol with no extent (or an extent with no target mapping) is an orphan
+    # from a failed alloc_image, manual cleanup or the v2 era. PVE's content
+    # delete (pvesm free, GUI Remove) and qm destroy call path() only for the
+    # owner/vtype, so dying here made such orphans undeletable (#292). Return
+    # a placeholder instead — activate_volume and qemu_blockdev_options still
+    # die on the same condition, so nothing can actually run against it.
+    my $ext = _find_extent($scfg, $volname);
+    unless ($ext && defined $ext->{target_id}) {
+        _log('warning', "path: $volname has no mapped iSCSI extent — returning placeholder path");
+        return (_unmapped_path($scfg, $volname), $ds_vmid, $vtype);
+    }
 
     # Look up the target that owns this extent (per-VM or legacy shared)
     my $t        = _api($scfg, 'GET', "/iscsi/target/id/$ext->{target_id}") // {};
@@ -627,7 +642,6 @@ sub path {
     my $portal   = _portal($scfg);
     my $dev_path = "iscsi://$portal/$iqn/$ext->{lun_id}";
 
-    my ($vtype, undef, $ds_vmid) = $class->parse_volname($volname);
     return ($dev_path, $ds_vmid, $vtype);
 }
 

perl5/PVE/Storage/Custom/TrueNASMultipath.pm

@@ -95,11 +95,17 @@ sub path {
 
     PVE::Storage::Custom::TrueNAS::_resolve_token($storeid, $scfg);
 
-    my $ext = PVE::Storage::Custom::TrueNAS::_find_extent($scfg, $volname);
-    die "Volume '$volname' has no iSCSI extent on $scfg->{truenas_host}.\n" unless $ext;
-
-    my $wwid = _naa_to_wwid($ext->{naa})
-        or die "Volume '$volname' has no NAA/WWID on $scfg->{truenas_host}.\n";
+    # Orphaned zvol (no extent / no NAA): return a placeholder with the real
+    # owner and vtype so pvesm free / GUI Remove can still delete it (#292).
+    # activate_volume dies on the same condition, so it is never opened.
+    my $ext  = PVE::Storage::Custom::TrueNAS::_find_extent($scfg, $volname);
+    my $wwid = $ext ? _naa_to_wwid($ext->{naa}) : undef;
+    unless ($wwid) {
+        PVE::Storage::Custom::TrueNAS::_log('warning',
+            "multipath path: $volname has no iSCSI extent/WWID — returning placeholder path");
+        my ($vtype, undef, $ds_vmid) = $class->parse_volname($volname);
+        return ("/dev/mapper/truenas-unmapped-$volname", $ds_vmid, $vtype);
+    }
 
     return ("/dev/mapper/$wwid", $ext->{lun_id}, $storeid);
 }

t/lib/MockTrueNAS.pm

@@ -0,0 +1,74 @@
+package MockTrueNAS;
+
+# In-memory fake of the TrueNAS API for unit tests. Replaces
+# PVE::Storage::Custom::TrueNAS::_api (and silences _log) so plugin entry
+# points can be exercised without a TrueNAS host or a PVE node.
+#
+# Usage:
+#   my $api = MockTrueNAS->install(
+#       extents       => [ { id => 1, name => 'vm-100-disk-0', naa => '0x6589cfc...' } ],
+#       targetextents => [ { id => 1, extent => 1, target => 7, lunid => 0 } ],
+#       targets       => [ { id => 7, name => 'proxmox-vm-100' } ],
+#       datasets      => { 'tank/vm/vm-100-disk-0' => { volsize => { parsed => 1024 } } },
+#   );
+#   ... call plugin ...
+#   $api->calls   # arrayref of [method, path, data]
+
+use strict;
+use warnings;
+
+use URI::Escape ();
+use PVE::Storage::Custom::TrueNAS;
+
+sub scfg {
+    return {
+        truenas_host    => 'truenas.test',
+        truenas_api_key => 'test-key',
+        truenas_pool    => 'tank',
+        truenas_dataset => 'vm',
+    };
+}
+
+sub install {
+    my ($class, %db) = @_;
+    my $self = bless { db => \%db, calls => [] }, $class;
+
+    no warnings 'redefine';    ## no critic (TestingAndDebugging::ProhibitNoWarnings)
+    *PVE::Storage::Custom::TrueNAS::_log = sub { push @{ $self->{log} }, [@_]; return; };
+    *PVE::Storage::Custom::TrueNAS::_api = sub { return $self->_route(@_); };
+
+    return $self;
+}
+
+sub calls { my ($self) = @_; return $self->{calls}; }
+sub log   { my ($self) = @_; return $self->{log} // []; }    ## no critic (Subroutines::ProhibitBuiltinHomonyms)
+
+sub _route {
+    my ($self, $scfg, $method, $path, $data) = @_;
+    push @{ $self->{calls} }, [ $method, $path, $data ];
+    my $db = $self->{db};
+
+    if ($method eq 'GET') {
+        return { basename => 'iqn.2005-10.org.freenas.ctl' } if $path eq '/iscsi/global';
+        return $db->{extents} // [] if $path eq '/iscsi/extent';
+
+        if ($path =~ m{^/iscsi/targetextent\?(extent|target)=(\d+)$}) {
+            my ($field, $id) = ($1, $2);
+            return [ grep { $_->{$field} == $id } @{ $db->{targetextents} // [] } ];
+        }
+        if ($path =~ m{^/iscsi/target/id/(\d+)$}) {
+            my ($t) = grep { $_->{id} == $1 } @{ $db->{targets} // [] };
+            return $t;
+        }
+        if ($path =~ m{^/pool/dataset/id/(.+)$}) {
+            my $name = URI::Escape::uri_unescape($1);
+            return $db->{datasets}{$name};
+        }
+    }
+
+    return if $method eq 'DELETE' || $method eq 'POST';
+
+    die "MockTrueNAS: unhandled $method $path\n";
+}
+
+1;

t/lib/PVE/Storage/Plugin.pm

@@ -0,0 +1,18 @@
+package PVE::Storage::Plugin;
+
+# Minimal stand-in for PVE::Storage::Plugin so the TrueNAS plugins can be
+# loaded and unit-tested off a Proxmox host. Only what the plugins call on
+# the base class at load time (or in tested code paths) is provided.
+
+use strict;
+use warnings;
+
+sub new          { return bless {}, shift; }
+sub register     { return; }
+sub lookup_types { return []; }
+sub properties   { return {}; }
+sub options      { return {}; }
+
+sub qemu_blockdev_options { return {}; }
+
+1;

t/path.t

@@ -0,0 +1,84 @@
+#!/usr/bin/perl
+# path() contract tests for both plugins. PVE expects ($path, $owner_vmid, $vtype);
+# callers like pvesm free, GUI Remove and qm destroy rely on the owner/vtype
+# even for volumes that cannot be opened.
+
+use strict;
+use warnings;
+
+use FindBin;
+use lib "$FindBin::Bin/lib", "$FindBin::Bin/../perl5";
+
+use Test::More;
+
+use MockTrueNAS;
+use PVE::Storage::Custom::TrueNAS;
+use PVE::Storage::Custom::TrueNASMultipath;
+
+my $TN = 'PVE::Storage::Custom::TrueNAS';
+my $MP = 'PVE::Storage::Custom::TrueNASMultipath';
+
+my %mapped = (
+    extents       => [ { id => 3, name => 'vm-100-disk-0', naa => '0x6589cfc000000abc' } ],
+    targetextents => [ { id => 5, extent => 3, target => 7, lunid => 2 } ],
+    targets       => [ { id => 7, name => 'proxmox-vm-100' } ],
+);
+
+subtest 'TrueNAS: mapped volume returns iscsi:// path' => sub {
+    MockTrueNAS->install(%mapped);
+    my ($path, $owner, $vtype) = $TN->path(MockTrueNAS::scfg(), 'vm-100-disk-0', 'store');
+    is($path, 'iscsi://truenas.test/iqn.2005-10.org.freenas.ctl:proxmox-vm-100/2', 'path');
+    is($owner, 100,      'owner is the VMID');
+    is($vtype, 'images', 'vtype');
+};
+
+# #292: an orphaned zvol (no extent) must still report owner/vtype so PVE can delete it.
+subtest 'TrueNAS: extent-less zvol returns placeholder, not die (#292)' => sub {
+    my $api = MockTrueNAS->install(extents => []);
+    my ($path, $owner, $vtype) = eval { $TN->path(MockTrueNAS::scfg(), 'vm-101-disk-1', 'store') };
+    is($@, '', 'does not die');
+    is($path, 'iscsi://truenas.test/unmapped/vm-101-disk-1', 'placeholder path');
+    is($owner, 101,      'owner is the VMID');
+    is($vtype, 'images', 'vtype');
+    ok((grep { $_->[0] eq 'warning' } @{ $api->log }), 'logs a warning');
+};
+
+subtest 'TrueNAS: extent with no target mapping returns placeholder (#292)' => sub {
+    MockTrueNAS->install(
+        extents       => [ { id => 3, name => 'vm-102-disk-0' } ],
+        targetextents => [],
+    );
+    my ($path, $owner) = eval { $TN->path(MockTrueNAS::scfg(), 'vm-102-disk-0', 'store') };
+    is($@, '', 'does not die');
+    like($path, qr{/unmapped/vm-102-disk-0$}, 'placeholder path');
+    is($owner, 102, 'owner is the VMID');
+};
+
+subtest 'TrueNAS: activate_volume still dies for extent-less zvol' => sub {
+    MockTrueNAS->install(extents => []);
+    eval { $TN->activate_volume('store', MockTrueNAS::scfg(), 'vm-101-disk-1') };
+    like($@, qr/has no iSCSI extent/, 'activate_volume dies');
+};
+
+subtest 'TrueNAS: qemu_blockdev_options still dies for extent-less zvol' => sub {
+    MockTrueNAS->install(extents => []);
+    eval { $TN->qemu_blockdev_options(MockTrueNAS::scfg(), 'store', 'vm-101-disk-1') };
+    like($@, qr/has no iSCSI extent/, 'qemu_blockdev_options dies');
+};
+
+subtest 'Multipath: extent-less zvol returns placeholder, not die (#292)' => sub {
+    MockTrueNAS->install(extents => []);
+    my ($path, $owner, $vtype) = eval { $MP->path(MockTrueNAS::scfg(), 'vm-990-disk-0', 'store') };
+    is($@, '', 'does not die');
+    is($path, '/dev/mapper/truenas-unmapped-vm-990-disk-0', 'placeholder path');
+    is($owner, 990,      'owner is the VMID');
+    is($vtype, 'images', 'vtype');
+};
+
+subtest 'Multipath: activate_volume still dies for extent-less zvol' => sub {
+    MockTrueNAS->install(extents => []);
+    eval { $MP->activate_volume('store', MockTrueNAS::scfg(), 'vm-990-disk-0') };
+    like($@, qr/has no iSCSI extent/, 'activate_volume dies');
+};
+
+done_testing();