[RFC PATCH storage] rbd: fix copy-offload flatten status race on fast small copies

[Ciro Iriarte <cyruspy@gmail.com>]

copy_image_status() read the image parent, then looked up the manager flatten
task, and declared the flatten dead ("no longer running and the image is still a
clone") whenever the task was missing. But the flatten task is only a liveness
signal; the image parent link is the completion signal. A fast flatten of a small,
near-empty image can vanish from `rbd task list` between the parent read and the
task lookup, so a copy that actually completed was misreported as a dead flatten
and the whole clone was rolled back.

This surfaced live on a running multi-disk clone (3 offloaded 2 GiB disks): one
disk flattened and was reaped cleanly while another, polled a moment later, hit the
empty-task window and failed the clone.

Re-read the authoritative parent when the task is missing: if the flatten just
finished the parent is now gone (complete); only when the parent persists AND a
second task look-up is still empty is the flatten really dead. A genuinely dead
flatten still fails as before.

Add copy_offload_rbd_status_test.pm, the first unit test on the rbd offload path:
it mocks the rbd/ceph command output to pin the race (empty task + parent cleared
on re-read => complete), the genuinely-dead case (=> dies), and the plain
running/complete states. It fails against the pre-fix code with the exact error.

Found by the freeze-bracket live-validation harness (pve-FCLUPlugin #22).

Generated-By: Claude (https://claude.ai)
Signed-off-by: Ciro Iriarte <ciro.iriarte+software@gmail.com>
Co-Authored-By: Claude <noreply@anthropic.com>
---
Applies on top of the v1 series; fixes the 2/5 rbd copy-offload. Found while
live-validating the freeze bracket on a running multi-disk clone (details in the
commit message). Not a v2 -- the rest of the series is unchanged.

 src/PVE/Storage/RBDPlugin.pm             | 117 +++++++++++++----------
 src/test/copy_offload_rbd_status_test.pm |  98 +++++++++++++++++++
 src/test/run_plugin_tests.pl             |   1 +
 3 files changed, 167 insertions(+), 49 deletions(-)
 create mode 100644 src/test/copy_offload_rbd_status_test.pm

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 31e9414..7f0dc00 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -919,61 +919,80 @@ sub copy_image_status {
 
     my (undef, $name) = $class->parse_volname($volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'info', $name, '--format', 'json');
-    my $info = '';
-    run_rbd_command($cmd, errmsg => "rbd info '$name' error", outfunc => sub { $info .= shift });
-
-    my $parent = eval { decode_json($info)->{parent} };
-
-    # No parent: the flatten finished and the image is INDEPENDENT -- which is the
-    # difference that matters. A clone is readable from the moment it exists, but
-    # deleting the source before the flatten completes would destroy it.
-    if (!$parent) {
-        # Drop the snapshot this copy was taken from, on the SOURCE image. Once the
-        # flatten completes the target no longer records where it came from, so this is
-        # the last chance to find it -- and leaving it behind pins space on the source
-        # and blocks deleting that volume later.
-        #
-        # Only ever our own: the name is derived from the target, so a caller-supplied
-        # snapshot can never match and is left untouched.
-        if ($source) {
-            my $own_snap = $rbd_copy_snap->($volname);
-            my (undef, $src_name) = $class->parse_volname($source->{volname});
-            eval {
-                my $c = $rbd_cmd->(
-                    $source->{scfg}, $source->{storeid},
-                    'snap', 'rm', $src_name, '--snap', $own_snap,
-                );
-                run_rbd_command($c, errmsg => "rbd snap rm error", outfunc => sub { });
-            };
-        }
-        # The placeholder copy_image_start() parked. Deliberately not removed there:
-        # 'rbd rm' walks every object and that call can run with a guest frozen. Best
-        # effort -- the copy is already complete and correct, and dying here would make
-        # the caller free it.
-        my $parked = copy_parked_name($name);
-        if (rbd_volume_exists($scfg, $storeid, $parked)) {
-            eval {
-                my $c = $rbd_cmd->($scfg, $storeid, 'rm', $parked);
-                run_rbd_command($c, errmsg => "rbd rm placeholder '$parked' error");
+    # The image's parent link is the source of truth for completion: no parent means the
+    # flatten finished and the image is INDEPENDENT -- the difference that matters. A
+    # clone is readable from the moment it exists, but deleting the source before the
+    # flatten completes would destroy it.
+    my $read_parent = sub {
+        my $cmd = $rbd_cmd->($scfg, $storeid, 'info', $name, '--format', 'json');
+        my $info = '';
+        run_rbd_command($cmd, errmsg => "rbd info '$name' error", outfunc => sub { $info .= shift });
+        return eval { decode_json($info)->{parent} };
+    };
+
+    my $parent = $read_parent->();
+
+    # Still a clone: the flatten is running, has just finished, or died. The manager task
+    # is only a LIVENESS signal, not the completion signal. A fast flatten of a small
+    # image can vanish from 'rbd task list' between the info read above and this task
+    # lookup, so a MISSING task is ambiguous -- re-read the authoritative parent before
+    # concluding the flatten died. Without this, a copy that actually completed is
+    # misreported as a dead flatten, the race that surfaces on concurrent multi-disk
+    # clones of small images.
+    if ($parent) {
+        my $task = $rbd_flatten_task->($scfg, $name);
+        if ($task) {
+            my $progress = $task->{progress};
+            return {
+                state => 'pending',
+                (defined($progress) ? (progress => int($progress * 100)) : ()),
             };
-            warn $@ if $@;
         }
 
-        return { state => 'complete' };
+        # No task: re-read the parent. If the flatten just finished it is now gone.
+        $parent = $read_parent->();
+        if ($parent) {
+            # Parent persists with no task. One more task look-up guards a transient
+            # empty 'rbd task list' while the flatten is genuinely still running; only
+            # when that is also empty is the flatten really dead.
+            die "flatten of '$volname' is no longer running and the image is still a clone\n"
+                if !$rbd_flatten_task->($scfg, $name);
+            return { state => 'pending' };
+        }
+        # Parent cleared -- the flatten completed; fall through to the completion cleanup.
     }
 
-    # Still parented. Distinguish a running flatten from one that died: without this a
-    # dead flatten leaves the image parented forever and the caller polls indefinitely.
-    my $task = $rbd_flatten_task->($scfg, $name);
-    die "flatten of '$volname' is no longer running and the image is still a clone\n"
-        if !$task;
+    # INDEPENDENT. Drop the snapshot this copy was taken from, on the SOURCE image. Once
+    # the flatten completes the target no longer records where it came from, so this is
+    # the last chance to find it -- and leaving it behind pins space on the source and
+    # blocks deleting that volume later.
+    #
+    # Only ever our own: the name is derived from the target, so a caller-supplied
+    # snapshot can never match and is left untouched.
+    if ($source) {
+        my $own_snap = $rbd_copy_snap->($volname);
+        my (undef, $src_name) = $class->parse_volname($source->{volname});
+        eval {
+            my $c = $rbd_cmd->(
+                $source->{scfg}, $source->{storeid},
+                'snap', 'rm', $src_name, '--snap', $own_snap,
+            );
+            run_rbd_command($c, errmsg => "rbd snap rm error", outfunc => sub { });
+        };
+    }
+    # The placeholder copy_image_start() parked. Deliberately not removed there: 'rbd rm'
+    # walks every object and that call can run with a guest frozen. Best effort -- the
+    # copy is already complete and correct, and dying here would make the caller free it.
+    my $parked = copy_parked_name($name);
+    if (rbd_volume_exists($scfg, $storeid, $parked)) {
+        eval {
+            my $c = $rbd_cmd->($scfg, $storeid, 'rm', $parked);
+            run_rbd_command($c, errmsg => "rbd rm placeholder '$parked' error");
+        };
+        warn $@ if $@;
+    }
 
-    my $progress = $task->{progress};
-    return {
-        state => 'pending',
-        (defined($progress) ? (progress => int($progress * 100)) : ()),
-    };
+    return { state => 'complete' };
 }
 
 sub alloc_image {
diff --git a/src/test/copy_offload_rbd_status_test.pm b/src/test/copy_offload_rbd_status_test.pm
new file mode 100644
index 0000000..d895f63
--- /dev/null
+++ b/src/test/copy_offload_rbd_status_test.pm
@@ -0,0 +1,98 @@
+package PVE::Storage::TestCopyOffloadRbdStatus;
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use PVE::Storage::RBDPlugin;
+use PVE::CephConfig;
+use Test::More;
+
+# copy_image_status() must treat the image PARENT LINK -- not the manager flatten task --
+# as the completion signal. A fast flatten of a small image can vanish from
+# `rbd task list` between the parent read and the task look-up, so a missing task while
+# the image still shows a parent is AMBIGUOUS: re-read the parent before declaring the
+# flatten dead. This pins that race (a copy that finished must not be reported as a dead
+# flatten -- the failure seen live on a concurrent multi-disk clone) plus the
+# unambiguous states around it.
+
+my $scfg = { type => 'rbd', pool => 'testpool' };
+my $storeid = 'rbd-a';
+my $volname = 'vm-100-disk-0';
+
+# Scripted command output, consumed in call order.
+my @info_json;      # what each 'rbd info' returns (parent present or not)
+my @task_json;      # what each 'ceph rbd task list' returns
+
+{
+    no warnings 'redefine';
+
+    # Keep $rbd_cmd from touching the ceph config / filesystem.
+    *PVE::CephConfig::ceph_connect_option = sub { return (); };
+
+    # Every rbd subcommand copy_image_status touches goes through run_rbd_command:
+    #   info  -> the scripted parent state
+    #   ls    -> '[]'  (rbd_volume_exists: the parked placeholder never exists here)
+    #   other -> succeed silently (snap rm / rm are best-effort cleanup)
+    *PVE::Storage::RBDPlugin::run_rbd_command = sub {
+        my ($cmd, %param) = @_;
+        my $op = join(' ', @$cmd);
+        my $out = $param{outfunc} // sub { };
+        if ($op =~ /\binfo\b/) {
+            $out->(shift(@info_json) // '{}');
+        } elsif ($op =~ /\bls\b/) {
+            $out->('[]');
+        }
+        return 0;
+    };
+
+    # $rbd_flatten_task runs `ceph rbd task list` through run_command.
+    *PVE::Storage::RBDPlugin::run_command = sub {
+        my ($cmd, %param) = @_;
+        my $out = $param{outfunc} // sub { };
+        $out->(shift(@task_json) // '[]');
+        return 0;
+    };
+}
+
+my $PARENT = '{"parent":{"pool":"testpool","image":"src","snapshot":"__copy_vm-100-disk-0"}}';
+my $NOPARENT = '{}';
+my $A_TASK =
+    '[{"refs":{"action":"flatten","image_name":"vm-100-disk-0","pool_name":"testpool"},"progress":0.5}]';
+my $NO_TASK = '[]';
+
+sub status {
+    return PVE::Storage::RBDPlugin->copy_image_status($scfg, $storeid, $volname, undef);
+}
+
+# 1. Independent on the first poll: no parent -> complete.
+@info_json = ($NOPARENT);
+@task_json = ();
+is(status()->{state}, 'complete', 'no parent on the first poll => complete');
+
+# 2. Flatten still running: parent + a live task -> pending (with progress).
+@info_json = ($PARENT);
+@task_json = ($A_TASK);
+my $st = status();
+is($st->{state}, 'pending', 'parent + a running flatten task => pending');
+is($st->{progress}, 50, '  progress surfaced from the task');
+
+# 3. THE RACE: parent seen, task list momentarily empty, re-read shows the flatten just
+#    finished. Must be complete, NOT a fatal "flatten no longer running".
+@info_json = ($PARENT, $NOPARENT);   # info#1 parent, info#2 (re-read) parent gone
+@task_json = ($NO_TASK);             # task look-up empty
+is(status()->{state}, 'complete',
+    'empty task + parent-cleared-on-reread => complete (the fixed race)');
+
+# 4. Genuinely dead flatten: parent persists across the re-read and no task on either
+#    look-up -> still fails, as before.
+@info_json = ($PARENT, $PARENT);
+@task_json = ($NO_TASK, $NO_TASK);   # first lookup + the guard lookup both empty
+my $died = !eval { status(); 1 };
+ok($died, 'parent persists + no task on both look-ups => dies (real failure still fails)');
+like($@, qr/no longer running and the image is still a clone/, '  with the flatten-dead error');
+
+done_testing();
+
+1;
diff --git a/src/test/run_plugin_tests.pl b/src/test/run_plugin_tests.pl
index 84995fe..0c55832 100755
--- a/src/test/run_plugin_tests.pl
+++ b/src/test/run_plugin_tests.pl
@@ -20,6 +20,7 @@ my $res = $harness->runtests(
     "copy_offload_test.pm",
     "copy_offload_naming_test.pm",
     "copy_offload_feature_test.pm",
+    "copy_offload_rbd_status_test.pm",
 );
 
 exit -1 if !$res || $res->{failed} || $res->{parse_errors};
-- 
2.54.0