From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 16B191FF09C for ; Mon, 05 Oct 2026 02:27:06 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id E771A21669; Mon, 05 Oct 2026 02:26:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=neatech-ar.20251104.gappssmtp.com; s=20251104; t=1791159980; x=1791764780; darn=lists.proxmox.com; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to:content-type; bh=EvQ7oFnqmiq3Rak61oZijaTp2dOyf2ICOrnCuF0OoIM=; b=Eqyx2S7R1BqkxgbC8oR3anWDZqxd12xw1fTUnF2XI85Ejf59TsLO7yVZFSkDuv0/Bl SUWBu2Qqpoc+vIZv1dxLoLzfQLZHP5S3UShUhGMP/m9taxio1NsBV3dpRtHqW3+iTbj/ 2u4IKTsVVLRzPx51vVPA+Ac+PEAZXrUz+otkbH5FN76cLzHF9ly2Qcung2shx+vlEbQS ptLOkKQTprDWDMzFkoxumAqygZF7TStPAqOAnHC6gn9JzPt+VrtxlwWJq0EWpXzIzEvI 8fCYHiaGQbDzr0Bv4L4KMfyJsGsabCV4+JtjL8NOe2J45KHOKs+OWP050+BUoomIH35A Yx6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1791159980; x=1791764780; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to:content-type; bh=EvQ7oFnqmiq3Rak61oZijaTp2dOyf2ICOrnCuF0OoIM=; b=GkV3mrD22QygHrmRblgHPtJeEgw5eE3JOFfTVM3iwrJliZarNEgLujxC/efIBzReyD mImQ4yHc0qhEY53MtkTSF/rUgWUKIgxITyxVHuJEvgJ6dreEq6KUbETQ52Z0TbZRE33X YVO30a5sEeSnamFI5yXQrmlXGav+b7q3lYRJd7wi/s1Vaj+3v5HI6/d/WGEt+AC/cGc/ xo8FwYD7MWbY80SheWQ8/h12xRWV9EfkwYIdHAg5+eQEsSQBtZW8ROrwO+54V0JeUDa0 CHbgf/8UolnNaJxN5HZSufZm0HAWctRMnHlkOirnK0/EdfLASp5skKp3lI3269DxqcQd YKiw== X-Gm-Message-State: AFq9FYIECJXuOcd5ifsAWDBfks8XT3nH1mcX8zQYfKMgZkVm1J30k3WP 3uFCl1ykWX8QtzuNN+P7V/cKMyjO5441lCqkl2XZY+kF1mN/8xKNkq+haYDipenr29KearulgvW hI1Zu4g4= X-Gm-Gg: AYBFou2UxfM+64xYFIJMFMf0gfoQqax6EMGtRMNVmaPTmAsm1k+1pXbB/dPwDjdks+f DtBgCWVJ6T3+ikyDZIcoVAcT4ZyYISBe0/FxOQEasH3dPZpMwWuJgB7Nb6lEhNPfsq6G8RVq2eV 6hSRGuawSKGL2LykmGz9D/zkZGYWPDsvp3Do1f+QRtGaVdLUZZ5q1QGGpyZOfneGhU1aFlJ75uD jNmOjH3VXd3STmHLpw3noNbOy3hYleMwa64Cx8Zj53+lrpUfqlgpFP7TtbqE7Gtd9JLnx9Nty+5 lZ6JSVwJZrX0stJB0nXNEP7E5+V4S8d1hCeyuXO9WpDlIwolxTUfIM7yCzSwdTEZPVGO2djmQYZ 77gQaMbknJH/TgJvMLvQkAW7CqFanC5EHaPrHzUtUysSQYcrSBqQ2FlF7j/Y9glgYpCEX/Zuc5w uC3A0yRoDjWMn6I9uUHSIZsxX00/C5OLqs66vIYFwygS1+BI54xKAuh8cqaCFCseXOUJ0RlghF8 DKXcklxK65gj8RNQvzPWq+Cl6sjf5UkMpmiUX8SLk7j0PCORsY9YkncitNX8MWd/KNLWxPj2dFc X-Received: by 2002:a05:6122:6b93:20b0:5e2:2a40:f2ea with SMTP id 71dfb90a1353d-5e22a41ef54mr263662e0c.3.1791159979235; Sun, 04 Oct 2026 17:26:19 -0700 (PDT) From: Joaquin Varela To: pve-devel@lists.proxmox.com Subject: [PATCH storage v3 3/4] zfsnvme: fence target commands of abandoned transactions Date: Sun, 4 Oct 2026 21:26:06 -0300 Message-ID: <20261005002609.571-4-joaquinvarela@neatech.ar> X-Mailer: git-send-email 2.54.0.windows.1 In-Reply-To: <20261005002609.571-1-joaquinvarela@neatech.ar> References: <20261005002609.571-1-joaquinvarela@neatech.ar> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 1 ADVANCE_FEE_3_NEW 1.5 Appears to be advance fee fraud (Nigerian 419) AWL -0.989 Adjusted score from AWL reputation of From: address DKIM_SIGNED 0.1 Message has a DKIM or DK signature, not necessarily valid DKIM_VALID -0.1 Message has at least one valid DKIM or DK signature DMARC_PASS -0.1 DMARC pass policy KAM_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: BCWJEIYET7CAYCZASY344JDEDREX6ICI X-Message-ID-Hash: BCWJEIYET7CAYCZASY344JDEDREX6ICI X-MailFrom: joaquinvarela@neatech.ar X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: ssh does not stop a command on the target when the client gives up on it. A zfs command that outlives its connection, a chain whose connection broke in the middle, or a call of a task that was stopped keeps running on the target after the plugin reported the outcome as unknown. The pmxcfs domain lock cannot see it: cfs_lock aborts the callback after 60 seconds and pmxcfs lets another node take the lock after 120 seconds, but neither stops the command on the target. The next transaction can then interleave its own commands with the rest of the abandoned one, and a late command of an old plan could change a zvol that a newer plan already replaced under the same name. Wrap every call of a transaction in a fixed POSIX sh guard that uses flock(1) on /run/pve-storage-nvmet/lock and an owner file next to it. Each transaction first claims the target: it reads the sha256 digest of the current owner file without the flock, then, under the flock, writes its own random token only if the owner still has that digest (compare-and-set), so an abandoned claim that reaches the target late cannot replace an owner that took over in between. Every later call of the transaction, reads included, takes the same flock and checks the token before its commands run. A command still running under the flock thus drains before a new owner can claim, and a command queued by a superseded owner exits with code 73 instead of running, as does a claim whose compare-and-set fails, which is reported as a target that another transaction took over. Every guarded call first checks the directory: if /run/pve-storage-nvmet is a symlink or not a root-owned directory with mode 0700, or its lock or owner entry is a symlink or exists as anything but a regular file, the call exits with code 74 before the flock and the error names that directory problem; only a call that carried a token is reported as superseded. A claim that cannot get the flock within 30 seconds exits with code 200 and is reported as "target busy", so a queue of transactions does not look like an unknown mutation outcome. Calls outside a transaction are sent unchanged and never wait for a long mutation. The guard is the only shell control flow on the target: a directory check, an if/else in the owner read, a command substitution for stat and one for the owner digest (kept in a variable, so a failing sha256sum is not taken for a match), and 'flock ... /bin/sh -c' around the unchanged command chains. It needs flock (util-linux), stat and sha256sum (coreutils) on the target. Chunking now sizes the wrapped call. A fenced call aborts the operation ("superseded by a newer one; the target state is unknown") and is never compensated; the next activation repairs from the target state as before. This is control-plane exclusion only: it fences stale commands of this plugin on the target, not hosts or guest I/O, which remain the job of HA and of the NVMe controller timeouts. The emulated target gains the owner, claim and token bookkeeping. New tests cover a callback fenced after its pmxcfs lock expired, a claim that timed out, a claim that finds another owner, a busy claim during alloc_image, unusable owner observations, an unusable lock directory, exit code 73 outside a transaction, the exit code classification of the runner, and the guard itself with /bin/sh in a temporary directory, which now also needs flock, stat and timeout. In the existing create test, the delayed create is now fenced instead of landing after the second allocation. Signed-off-by: Joaquin Varela --- src/PVE/Storage/ZFSNVMePlugin.pm | 159 ++++++++++- src/test/zfsnvme_target_test.pm | 476 ++++++++++++++++++++++++++++++- src/test/zfsnvme_test.pm | 43 +++ 3 files changed, 652 insertions(+), 26 deletions(-) diff --git a/src/PVE/Storage/ZFSNVMePlugin.pm b/src/PVE/Storage/ZFSNVMePlugin.pm index 127680fb..dfd43d43 100644 --- a/src/PVE/Storage/ZFSNVMePlugin.pm +++ b/src/PVE/Storage/ZFSNVMePlugin.pm @@ -36,6 +36,13 @@ use base qw(PVE::Storage::Plugin); # commands, sometimes joined with `&&`, to read or change ZFS/configfs state. # A pmxcfs domain lock per target serializes the changes within the cluster, # held like the storage lock of the other shared storage types. +# +# A fixed POSIX sh transport guard also uses a target flock and owner token: +# a transaction claims the target by comparing the observed predecessor +# before publishing its token, and its later calls, reads included, check +# that token under the flock. Observations outside transactions and the +# pre-claim owner observation bypass the flock. The guard excludes stale +# control-plane commands, not guest I/O or hosts. # --------------------------------------------------------------------------- # Constants and regular expressions @@ -60,6 +67,10 @@ my $nvmet_max_command = 65536; # Seconds to wait for the target lock while another node changes the target; # a destroy or rollback can hold it for several seconds. my $nvmet_lock_wait = 30; +my $nvmet_lock_dir = '/run/pve-storage-nvmet'; +my $nvmet_fenced = 73; # a superseded token, or a claim whose compare-and-set failed +my $nvmet_unsafe_dir = 74; # $nvmet_lock_dir cannot be used as the lock directory +my $nvmet_busy = 200; # reserved for lock acquisition during a claim, never a storage command # On the pool dataset: the highest NSID handed out for a volume of the pool. my $nvmet_last_nsid = 'proxmox:nvme-last-nsid'; @@ -77,8 +88,9 @@ my @nvmet_cfs_attrs = qw( attr_model attr_serial attr_allow_any_host addr_trtype addr_adrfam addr_traddr addr_trsvcid ); -# Every command the plugin runs on the target, besides the `dd | tee` pipe of -# a key write. The configfs read runs find through env to set its locale. +# Allowlist for ordinary steps. The configfs read runs find through env to set +# its locale; key writes use a dedicated `dd | tee` pipe. The transport guard +# additionally uses POSIX sh, flock, stat, sha256sum and shell builtins. my %nvmet_commands = map { $_ => 1 } qw( cat chmod env grep ln mkdir modprobe mount printf rm rmdir test zfs ); @@ -143,6 +155,8 @@ my $RE_BASE_SNAPSHOT = qr{^ (?\S+) \@__base__ $}nxx; my $RE_UNSIGNED_INTEGER = qr{^ (?\d+) $}nxx; my $RE_NVMET_UUID = qr{\A [0-9a-fA-F]{8} (?: - [0-9a-fA-F]{4}){3} - [0-9a-fA-F]{12} \z}nxx; +my $RE_NVMET_OWNER_DIGEST = qr{\A [0-9a-f]{64} \z}nxx; +my $RE_NVMET_OWNER_SNAPSHOT = qr{\A (?[0-9a-f]{64}) \x20{2} - \z}nxx; my $RE_NVMET_NSID = qr{\A [1-9] [0-9]* \z}nxx; my $RE_NVMET_POOL = qr{\A [A-Za-z0-9] [A-Za-z0-9_.:/-]* \z}nxx; my $RE_NVMET_DATASET_NAME = qr{\A [A-Za-z0-9] [A-Za-z0-9_.:-]* \z}nxx; @@ -469,7 +483,8 @@ my sub nvmet_quote($word) { # Renders steps into one POSIX shell command line: simple commands joined with # `&&`, plus `>` for configfs writes and one `dd | tee` pipe for a key read from -# stdin. There are no loops, variables, conditionals or substitutions. Every +# stdin. This renderer has no loops, variables, conditionals or substitutions +# in its output; _nvmet_render_call adds the separate transport guard. Every # word is quoted; all operands are built from validated configuration. sub _nvmet_render($steps) { my $invalid = "internal error: invalid NVMe target step\n"; @@ -514,16 +529,19 @@ sub _nvmet_render($steps) { return join(' && ', @commands); } -# Packs whole units into calls of at most $max bytes of rendered command, the -# single argument sshd runs, below its limit. A rendered command is ASCII -# (every operand is validated), so its length is its size in bytes. +# Packs whole units into calls of at most $max bytes, including the transport +# guard and the outer shell quoting, below sshd's single-argument limit. A +# rendered command is ASCII (every operand is validated), so its length is +# its size in bytes. sub _nvmet_chunk($units, $max = $nvmet_max_command) { my (@chunks, @current); + my $sizing_token = '00000000-0000-4000-8000-000000000000'; for my $unit ($units->@*) { my @next = (@current, $unit->@*); - if (length(_nvmet_render(\@next)) > $max) { + if (length(_nvmet_render_call(\@next, token => $sizing_token)) > $max) { die "internal error: NVMe target command too long\n" - if !@current || length(_nvmet_render($unit)) > $max; + if !@current + || length(_nvmet_render_call($unit, token => $sizing_token)) > $max; push @chunks, [@current]; @next = $unit->@*; } @@ -533,6 +551,78 @@ sub _nvmet_chunk($units, $max = $nvmet_max_command) { return \@chunks; } +# Transport fencing, not target-side storage logic. The fixed target lock is +# inherited by sh and its children, so a command outliving SSH still excludes +# the next owner. Token checks reject commands queued by a superseded owner. +# Claims compare their observed predecessor, so an abandoned claim cannot +# replace an owner which took over after that observation. +sub _nvmet_render_call($steps, %opts) { + # Observations outside a transaction must not wait for a long-running + # mutation. Transactional reads still carry a token and take the lock. + return _nvmet_render($steps) + if !$opts{read_owner} && !defined($opts{claim}) && !defined($opts{token}); + + my $dir = nvmet_quote($nvmet_lock_dir); + my $lock = nvmet_quote("$nvmet_lock_dir/lock"); + my $owner = nvmet_quote("$nvmet_lock_dir/owner"); + my $token = $opts{claim} // $opts{token}; + die "internal error: invalid NVMe target token\n" + if defined($token) && $token !~ $RE_NVMET_UUID; + # /run is root-owned. Never follow a pre-existing symlink or reuse an + # incorrectly owned/mode directory; never unlink the lock inode. A + # directory that cannot be used is a target problem, not a fencing verdict. + my $prepare = + "umask 077; (mkdir -m 0700 $dir 2>/dev/null || test -d $dir)" + . " && test ! -L $dir && test -d $dir" + . " && test \"\$(stat -c '%u:%a' $dir)\" = '0:700'" + . " && test ! -L $lock && (test ! -e $lock || test -f $lock)" + . " && test ! -L $owner && (test ! -e $owner || test -f $owner)" + . " || exit $nvmet_unsafe_dir; "; + if ($opts{read_owner}) { + die "internal error: NVMe owner observation has commands or a token\n" + if $steps->@* || defined($token); + # A failed hash/read is not an absent owner. The digest also represents + # empty files and arbitrary non-token content without interpreting + # either as a valid transaction token. + return $prepare + . "if test -e $owner; then sha256sum < $owner; else printf '%s\\n' missing; fi"; + } + my $body; + if (defined($opts{claim})) { + die "internal error: NVMe target claim has commands\n" if $steps->@*; + my $expected = $opts{expected_owner}; + die "internal error: invalid NVMe target predecessor\n" + if !defined($expected) + || ($expected ne 'missing' && $expected !~ $RE_NVMET_OWNER_DIGEST); + my $compare = + $expected eq 'missing' + ? "test ! -e $owner" + : "current=\$(sha256sum < $owner) && test \"\$current\" = " + . nvmet_quote("$expected -"); + # The claim has no storage commands. Normalize its only operation's + # failure so it cannot be mistaken for flock's conflict exit code. + $body = + "$compare || exit $nvmet_fenced; printf '%s\\n' " + . nvmet_quote($token) + . " > $owner || exit 1"; + } else { + $body = _nvmet_render($steps); + $body = 'grep -Fqx -- ' . nvmet_quote($token) . " $owner || exit $nvmet_fenced; $body" + if defined($token); + } + my $conflict = defined($opts{claim}) ? "-E $nvmet_busy " : ''; + return + $prepare + . "flock -x -w $nvmet_lock_wait $conflict$lock /bin/sh -c " + . nvmet_quote($body); +} + +sub _nvmet_new_token() { + my $token = lc(file_read_firstline('/proc/sys/kernel/random/uuid') // ''); + die "cannot generate NVMe target transaction token\n" if $token !~ $RE_NVMET_UUID; + return $token; +} + # Never propagate the context run_command adds to the task marker: it quotes # the command line. my sub rethrow_task_interrupt($error) { @@ -543,10 +633,16 @@ my sub rethrow_task_interrupt($error) { # { rc => exit code, or -1 when ssh did not run to its end, out => [lines], # err => text } and never dies on a failing command; a stopped task dies with # the task marker. %opts: op (label, required), timeout and input (stdin, -# used for keys). +# used for keys); the transport guard options of _nvmet_render_call: +# read_owner, claim with expected_owner, and token. A guarded call that +# finds the lock directory unusable (exit $nvmet_unsafe_dir) gets the +# directory problem as its error text; a claim that could not acquire the +# flock (exit $nvmet_busy) is reported as target busy, and one whose +# compare-and-set failed (exit $nvmet_fenced) as a target that another +# transaction took over, which a repeated operation claims anew. sub _nvmet_run($scfg, $steps, %opts) { die "internal error: NVMe target call without label\n" if !defined($opts{op}); - my $command = _nvmet_render($steps); + my $command = _nvmet_render_call($steps, %opts); die "internal error: NVMe target command too long\n" if length($command) > $nvmet_max_command; my $cmd = [@ssh_cmd, '-i', nvmet_ssh_key($scfg), 'root@' . nvmet_server($scfg), $command]; my (@out, $err); @@ -568,6 +664,15 @@ sub _nvmet_run($scfg, $steps, %opts) { return { rc => -1, out => [], err => 'ssh failed' } if $error !~ $RE_COMMAND_EXIT; $rc = $+{code}; } + $err = + "unsafe lock directory $nvmet_lock_dir on the target: it must be a root-owned" + . " directory with mode 0700 whose lock and owner entries are regular files" + if ($opts{read_owner} || defined($opts{claim}) || defined($opts{token})) + && $rc == $nvmet_unsafe_dir; + $err = 'target busy: could not acquire transaction lock' + if defined($opts{claim}) && $rc == $nvmet_busy; + $err = 'another transaction took over the target' + if defined($opts{claim}) && $rc == $nvmet_fenced; return { rc => $rc, out => \@out, err => $err // ($rc ? "exit code $rc" : '') }; } @@ -1096,10 +1201,12 @@ sub _nvmet_plan_orphan_hosts($cfs, $candidates) { # --------------------------------------------------------------------------- my %nvmet_lock_owner; # lock id => pid of the process holding the domain lock +my %nvmet_lock_token; # lock id => target-side transaction token # Runs $code under the pmxcfs domain lock of the target. Like the pmxcfs # lock itself, it is not re-entrant, and a child forked inside is not the -# owner. +# owner. pmxcfs can break a lock after 120 seconds: the target flock drains +# an executing command before a new token fences all calls of the old owner. my sub nvmet_locked($scfg, $code) { my $id = nvmet_lock_id($scfg); die "cluster not quorate - refusing NVMe target changes\n" @@ -1108,7 +1215,29 @@ my sub nvmet_locked($scfg, $code) { $id, $nvmet_lock_wait, sub { + my $transaction = _nvmet_new_token(); + my $snapshot = _nvmet_run( + $scfg, [], + read_owner => 1, + op => 'read target transaction owner', + ); + die "cannot observe NVMe target transaction owner: $snapshot->{err}\n" + if $snapshot->{rc}; + my $line = $snapshot->{out}->[0] // ''; + die "invalid NVMe target transaction owner observation\n" + if $snapshot->{out}->@* != 1 + || ($line ne 'missing' && $line !~ $RE_NVMET_OWNER_SNAPSHOT); + my $expected = $line eq 'missing' ? $line : $+{digest}; + my $claim = _nvmet_run( + $scfg, [], + claim => $transaction, + expected_owner => $expected, + op => 'claim target transaction', + timeout => $nvmet_lock_wait + 15, + ); + die "cannot claim NVMe target transaction: $claim->{err}\n" if $claim->{rc}; local $nvmet_lock_owner{$id} = $$; + local $nvmet_lock_token{$id} = $transaction; return $code->(); }, ); @@ -1124,8 +1253,9 @@ my sub nvmet_locked($scfg, $code) { # did not run to its end (-1: its timeout, or ssh was killed), and a call # during which the task was stopped (it dies with the task marker) may still # be running there. Its outcome is unknown, so it is never compensated from a -# read that could come before the rest of it. The next activation repairs -# from the target state. +# read that could come before the rest of it. The next activation, under a +# new token, repairs from the target state. A same-token read cannot settle a +# lost reply: it could overtake the abandoned command before flock. my sub nvmet_exec($scfg, $steps, %opts) { my $locked = ($nvmet_lock_owner{ nvmet_lock_id($scfg) } // 0) == $$; die "internal error: NVMe target change without target lock\n" @@ -1134,8 +1264,11 @@ my sub nvmet_exec($scfg, $steps, %opts) { $scfg, $steps, op => $opts{op}, timeout => $opts{timeout} // 15, + ($locked ? (token => $nvmet_lock_token{ nvmet_lock_id($scfg) }) : ()), (defined($opts{input}) ? (input => $opts{input}) : ()), ); + die "NVMe target transaction was superseded by a newer one; the target state is unknown\n" + if $locked && $res->{rc} == $nvmet_fenced; die "NVMe target operation '$opts{op}' did not complete ($res->{err});" . " the target state is unknown\n" if ($locked && $res->{rc} == -1) diff --git a/src/test/zfsnvme_target_test.pm b/src/test/zfsnvme_target_test.pm index 991d2084..1b109bfd 100644 --- a/src/test/zfsnvme_target_test.pm +++ b/src/test/zfsnvme_target_test.pm @@ -9,6 +9,7 @@ use lib qw(..); use Compress::Zlib qw(crc32); use Digest::SHA qw(sha256_hex); +use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC LOCK_EX LOCK_NB); use File::Temp qw(tempdir); use FindBin; use IPC::Open3; @@ -112,6 +113,7 @@ our (%LOCK_HELD, @LOCKS, @NESTED_LOCKS, @QUORUM, @WARNINGS, @SYSFS_WRITES, %FILE our (%CORPUS, @VIOLATIONS, @WRITES, @UNLINKED, %BLOCK, @UUIDS); my $uuid_seq = 0; +my $token_seq = 0; sub lock_id($scfg) { return 'zfsnvme-' . ($scfg->{server} =~ s/[^A-Za-z0-9.-]/_/gr); @@ -145,11 +147,18 @@ my $tools_mock = Test::MockModule->new('PVE::Tools'); $plugin_mock->redefine( _nvmet_run => sub($scfg, $steps, %opts) { die "test error: no fake target\n" if !$FAKE; + return $FAKE->read_owner() if $opts{read_owner}; + return $FAKE->claim($opts{claim}, %opts) if defined($opts{claim}); my $rendered = nv('_nvmet_render', $steps); $CORPUS{$rendered} //= $opts{op} // ''; return $FAKE->run($scfg, $steps, $rendered, %opts); }, ); +$plugin_mock->redefine( + _nvmet_new_token => sub () { + return sprintf('ffffffff-ffff-4000-8000-%012x', ++$token_seq); + }, +); # The target is the only place that runs commands, and only through _nvmet_run. # A command is also a violation, in case the caller handles the error. my $no_command = sub($cmd, %opts) { @@ -292,6 +301,11 @@ package FakeTarget { violations => [], faults => {}, # call index => before | after | late | cut: | lost: late => [], # calls that ssh gave up on, applied by land() + claims => [], + owner => undef, + owner_read => undef, # the answer of the owner observation, if not the owner + unsafe_dir => 0, # the lock directory of the target is unusable + fenced => 0, # every command exits with the fencing code # { after => call index, mode => before | unreachable | failed } read_fault => undef, step_fault => undef, # sub ($step, $call) returning an error text @@ -398,6 +412,61 @@ package FakeTarget { # --- execution -------------------------------------------------------- + sub owner_digest($self) { + return 'missing' if !defined($self->{owner}); + return sha256_hex("$self->{owner}\n"); + } + + # What _nvmet_run returns for a guarded call that found the lock directory + # unusable. + sub unsafe_dir_answer($self) { + return { + rc => 74, + out => [], + err => 'unsafe lock directory /run/pve-storage-nvmet on the target: it must be' + . ' a root-owned directory with mode 0700 whose lock and owner entries are' + . ' regular files', + }; + } + + sub read_owner($self) { + return $self->unsafe_dir_answer if $self->{unsafe_dir}; + return $self->{owner_read} if $self->{owner_read}; + my $owner = $self->owner_digest; + return { rc => 0, out => [$owner eq 'missing' ? $owner : "$owner -"], err => '' }; + } + + sub claim($self, $token, %opts) { + push $self->{claims}->@*, $token; + my $expected = $opts{expected_owner}; + $self->violation('claim without expected owner') if !defined($expected); + $self->violation("invalid expected owner '$expected'") + if $expected ne 'missing' && $expected !~ /\A[0-9a-f]{64}\z/; + return $self->unsafe_dir_answer if $self->{unsafe_dir}; + return { + rc => 200, + out => [], + err => 'target busy: could not acquire transaction lock', + } + if $self->{claim_busy}; + if ($self->{claim_fault}) { + $self->{late_claim} = { token => $token, expected_owner => $expected }; + return { rc => -1, out => [], err => 'timeout' }; + } + # A command already inside flock finishes before another owner can + # claim. Commands delayed before their guard instead see the new token. + $self->land(1); + return { rc => 73, out => [], err => 'another transaction took over the target' } + if $self->owner_digest ne $expected; + $self->{owner} = $token; + return { rc => 0, out => [], err => '' }; + } + + sub replay_late_claim($self) { + my $late = $self->{late_claim} // $self->violation('no late claim'); + return $self->claim($late->{token}, expected_owner => $late->{expected_owner}); + } + sub run($self, $scfg, $steps, $rendered, %opts) { my $call = { index => scalar($self->{calls}->@*), @@ -410,6 +479,7 @@ package FakeTarget { mutating => (grep { main::mutating_step($_) } $steps->@*) ? 1 : 0, changing => main::changing($steps), now => $main::NOW, + token => $opts{token}, }; push $self->{calls}->@*, $call; $self->audit($call, $opts{input}); @@ -452,6 +522,8 @@ package FakeTarget { if defined($input) && !grep { $input eq "$_\n" } $self->{secrets}->@*; push @bad, "target change without the target lock in '$op'" if $call->{mutating} && !$call->{locked}; + push @bad, "transaction call without a fencing token in '$op'" + if $call->{locked} && !defined($call->{token}); $self->flag(@bad); } @@ -465,12 +537,22 @@ package FakeTarget { # Applies the calls that ssh gave up on, as the target finally runs them: # each chain stops at its first failing step. - sub land($self) { + sub land($self, $active_only = 0) { + my @pending; for my $late (splice($self->{late}->@*)) { + if ($active_only && !$late->{guarded}) { + push @pending, $late; + next; + } + next + if !$late->{guarded} + && defined($late->{token}) + && ($self->{owner} // '') ne $late->{token}; for my $step ($late->{steps}->@*) { last if !eval { $self->step($step, $late->{input}); 1 }; } } + push $self->{late}->@*, @pending; return $self; } @@ -480,6 +562,7 @@ package FakeTarget { # still runs, later), or ssh gives up on it while it runs to its end later # (late: -1). sub execute($self, $call, $steps, $input) { + $self->land(1) if defined($call->{token}); # only transactions drain active commands my $closed = 'Connection to 192.0.2.10 closed by remote host.'; my $broken = { rc => 255, out => [], err => $closed }; my $no_route = 'ssh: connect to host 192.0.2.10: No route to host'; @@ -497,9 +580,18 @@ package FakeTarget { $call->{fault} = $mode if $mode; return { rc => 1, out => [], err => 'injected failure' } if $mode eq 'before'; if ($mode eq 'late') { - push $self->{late}->@*, { steps => dclone($steps), input => $input }; + push $self->{late}->@*, + { + steps => dclone($steps), + input => $input, + token => $call->{token}, + }; return { rc => -1, out => [], err => 'timeout' }; } + return $self->unsafe_dir_answer if $self->{unsafe_dir} && defined($call->{token}); + return { rc => 73, out => [], err => 'transaction fenced' } + if $self->{fenced} + || (defined($call->{token}) && ($self->{owner} // '') ne $call->{token}); my ($cut) = $mode =~ /\Acut:([0-9]+)\z/; my ($lost) = $mode =~ /\Alost:([0-9]+)\z/; my @out; @@ -507,7 +599,13 @@ package FakeTarget { return $broken if defined($cut) && $index == $cut; if (defined($lost) && $index == $lost) { my @rest = $steps->@[$index .. $steps->$#*]; - push $self->{late}->@*, { steps => dclone(\@rest), input => $input }; + push $self->{late}->@*, + { + steps => dclone(\@rest), + input => $input, + token => $call->{token}, + guarded => 1, + }; return $broken; } my $step = $steps->[$index]; @@ -2544,7 +2642,7 @@ subtest 'template name and chunking' => sub { my $chunks = nv('_nvmet_chunk', \@units); ok($chunks->@* > 1, 'large plans are split'); ok( - !grep({ length(nv('_nvmet_render', $_)) > 65536 } $chunks->@*), + !grep({ length(nv('_nvmet_render_call', $_, token => $U{1})) > 65536 } $chunks->@*), 'no call exceeds 64 KiB', ); ok(!grep({ $_->[0]->[0] ne 'test' || $_->@* % 6 } $chunks->@*), 'chunks hold whole units'); @@ -2553,7 +2651,7 @@ subtest 'template name and chunking' => sub { my $small = nv( '_nvmet_chunk', [@units[0 .. 3]], - length(nv('_nvmet_render', [map { $_->@* } @units[0 .. 1]])), + length(nv('_nvmet_render_call', [map { $_->@* } @units[0 .. 1]], token => $U{1})), ); is(scalar($small->@*), 2, 'the limit is configurable'); eval { nv('_nvmet_chunk', [$units[0]], 10) }; @@ -3206,14 +3304,12 @@ subtest 'create' => sub { $fake->land; is_deeply( [map { owned_volumes($fake->{m})->{"tank/vm-$_-disk-0"}->[0] } 200, 201], - [4, 5], - 'never gets the NSID of the create that completes afterwards', + [undef, 5], + 'fences the delayed create and never reuses its reserved NSID', ); $res = flow($fake, $ACT{activate}); - ok( - !$res->{error} && ns_of($fake, 4) && ns_of($fake, 5), - 'and the next activation exports both', - ); + ok(!$res->{error} && !ns_of($fake, 4) && ns_of($fake, 5), + 'only the second one is exported'); ($fake, $res) = run_on( 'alloc', @@ -3664,6 +3760,66 @@ subtest 'resize' => sub { is($fake->{m}->{ds}->{'tank/vm-100-disk-0'}->{volsize}, 2 * 1024**3, 'keeps the new size'); }; +subtest 'a new owner fences a still-running cluster-lock callback' => sub { + my $fake = lifecycle_fake(); + my $superseded = 0; + $fake->{before_call} = sub($f, $call) { + return if $call->{op} ne 'resize zvol' || $superseded++; + $NOW += 121; + local %LOCK_HELD; # pmxcfs permits the expired lease to be replaced + my $new = + flow($f, sub { $PLUGIN->volume_resize(scfg(), 'st', 'vm-100-disk-0', 3 * 1024**3) }); + is($new->{error}, '', 'a second owner completes its fresh plan'); + }; + my $res = flow($fake, $ACT{resize}); + like( + $res->{error}, + qr/transaction was superseded by a newer one; the target state is unknown\n\z/, + 'the old callback cannot resume', + ); + is($fake->{m}->{ds}->{'tank/vm-100-disk-0'}->{volsize}, 3 * 1024**3, 'the new size stays'); + is(scalar($fake->{claims}->@*), 2, 'each callback claims only once'); +}; + +subtest 'an uncertain claim never starts a transaction' => sub { + my $fake = lifecycle_fake(claim_fault => 1); + my $before = dclone($fake->{m}); + my $res = flow($fake, $ACT{resize}); + like($res->{error}, qr/cannot claim NVMe target transaction/, 'claim timeout aborts'); + is( + scalar($fake->{calls}->@*), + 0, + 'no planning read or mutation follows the uncertain claim', + ); + is_deeply($fake->{m}, $before, 'storage state is unchanged'); + $fake->{claim_fault} = 0; + is($fake->replay_late_claim->{rc}, 0, 'the abandoned claim can still reach the target'); + is($fake->{owner}, $fake->{late_claim}->{token}, 'the late claim publishes its token'); + is_deeply($fake->{m}, $before, 'the accepted late claim changes no storage state'); + my $next = + flow($fake, sub { $PLUGIN->volume_resize(scfg(), 'st', 'vm-100-disk-0', 3 * 1024**3) }); + is($next->{error}, '', 'a fresh transaction claims the late owner by its digest'); + isnt($fake->{owner}, $fake->{late_claim}->{token}, 'the fresh transaction replaces it'); + is($fake->{m}->{ds}->{'tank/vm-100-disk-0'}->{volsize}, 3 * 1024**3, 'with its own resize'); +}; + +subtest 'a claim that finds another owner' => sub { + # the owner changed between the observation and the claim + my ($fake, $res) = run_on( + 'resize', + owner => $U{2}, + owner_read => { rc => 0, out => [sha256_hex("$U{1}\n") . ' -'], err => '' }, + ); + is( + $res->{error}, + "cannot claim NVMe target transaction: another transaction took over the target\n", + 'reports the takeover', + ); + is_deeply([scalar($fake->{claims}->@*), $res->{calls}], [1, []], 'after the one claim'); + is($fake->{owner}, $U{2}, 'the owner that took over stays'); + is($fake->{m}->{ds}->{'tank/vm-100-disk-0'}->{volsize}, 1024**3, 'and nothing changed'); +}; + sub removal_fake(%opts) { my $fake = target_fake(%opts); $fake->add_zvol('other/foreign-disk', identity => [$FOREIGN_NQN, 1, $U{8}]); @@ -3876,6 +4032,90 @@ subtest 'snapshots and the generic ZFS calls' => sub { } }; +subtest 'a busy claim prevents public allocation' => sub { + my $fake = lifecycle_fake(claim_busy => 1, owner => $U{1}); + my $before = dclone($fake->{m}); + my $res = flow( + $fake, sub { $PLUGIN->alloc_image('st', scfg(), 200, 'raw', 'vm-200-disk-0', 1024) }, + ); + is( + $res->{error}, + "cannot claim NVMe target transaction: target busy: could not acquire transaction lock\n", + 'public allocation reports busy, not an unknown mutation outcome', + ); + is(scalar($fake->{claims}->@*), 1, 'the claim is attempted once'); + is_deeply($res->{calls}, [], + 'no planning, mutation or compensation follows the busy claim'); + is($fake->{owner}, $U{1}, 'the existing owner is unchanged'); + is_deeply($fake->{m}, $before, 'storage state is unchanged'); +}; + +subtest 'an unusable owner snapshot never starts a claim' => sub { + for my $case ( + [ + 'a timeout', + { rc => -1, out => [], err => 'timeout' }, + qr/cannot observe NVMe target transaction owner: timeout/, + ], + [ + 'a failed hash with matching stdout', + { rc => 1, out => [sha256_hex("owner\n") . ' -'], err => 'Input/output error' }, + qr/cannot observe NVMe target transaction owner: Input\/output error/, + ], + [ + 'a malformed snapshot', + { rc => 0, out => ['not an owner snapshot'], err => '' }, + qr/invalid NVMe target transaction owner observation/, + ], + ) { + my ($name, $snapshot, $error) = $case->@*; + my ($fake, $res) = run_on('resize', owner_read => $snapshot); + like($res->{error}, $error, "$name aborts the transaction"); + is_deeply([$fake->{claims}->@*, $fake->{calls}->@*], [], 'without a claim or a call'); + } +}; + +subtest 'an unusable lock directory on the target' => sub { + my ($fake, $res) = run_on('resize', unsafe_dir => 1); + is( + $res->{error}, + 'cannot observe NVMe target transaction owner: ' + . $fake->unsafe_dir_answer->{err} . "\n", + 'the owner observation names the directory problem', + ); + is_deeply([$fake->{claims}->@*, $fake->{calls}->@*], [], 'without a claim or a call'); + + # the directory becomes unusable while a transaction runs + $fake = lifecycle_fake( + before_call => sub($f, $call) { $f->{unsafe_dir} = 1 if $call->{op} eq 'resize zvol' }, + ); + $res = flow($fake, $ACT{resize}); + is( + $res->{error}, + "NVMe target operation 'resize zvol' failed: " . $fake->unsafe_dir_answer->{err} . "\n", + 'a later call reports the directory problem, not a superseded transaction', + ); + is($fake->{m}->{ds}->{'tank/vm-100-disk-0'}->{volsize}, 1024**3, 'and changed nothing'); +}; + +subtest 'exit code 73 is a fencing verdict only for a call with a token' => sub { + my $fake = lifecycle_fake(fenced => 1); + my $res = flow($fake, sub { [$PLUGIN->path(scfg(), 'vm-100-disk-0', 'st')] }); + is( + $res->{error}, + "cannot read NVMe target state: transaction fenced\n", + 'a read outside a transaction reports a failed command', + ); + is(scalar($res->{locks}->@*), 0, 'it ran without the lock'); + $res = flow($fake, $ACT{resize}); + is( + $res->{error}, + "NVMe target transaction was superseded by a newer one; the target state is unknown\n", + 'the same exit code fences the read of a transaction', + ); + is_deeply(ops($res), ['read target state'], 'which ends at that read'); +}; + subtest 'lock ownership' => sub { my $fake = lifecycle_fake(); pipe(my $reader, my $writer) or die "pipe: $!\n"; @@ -4595,7 +4835,7 @@ sub sh($command, $input = undef) { # The subtests that run rendered commands need these tools. A Debian build # has them, so a missing one is an error, never a silently skipped test. { - my @tools = qw(find grep sha256sum dd tee); + my @tools = qw(find grep sha256sum dd tee flock stat timeout); BAIL_OUT('needs /bin/sh with ' . join(', ', @tools)) if !-x '/bin/sh' || (sh('command -v ' . join(' ', @tools)))[0] != 0; } @@ -4813,9 +5053,219 @@ subtest 'the rendered chains on a configfs stand-in' => sub { ok(-d "$root/ports/1" && -d "$root/ports/2", 'ports stay'); }; +subtest 'transport guard executes with POSIX sh in an isolated directory' => sub { + my $dir = tempdir(CLEANUP => 1); + my $runtime = "$dir/runtime"; + my $output = "$dir/output"; + my $digest = sub($bytes) { return sha256_hex($bytes) }; + my $ownership = $> . ':700'; + my ($signal_rc) = sh('kill -TERM $$'); + is($signal_rc, 143, 'a signaled shell is not reported as a successful command'); + my $run = sub($steps, %opts) { + my $short_wait = delete $opts{test_short_wait}; + my $printf_failure = delete $opts{test_printf_failure}; + my $sha256_failure = delete $opts{test_sha256_failure}; + my $command = nv('_nvmet_render_call', $steps, %opts); + is(system('/bin/sh', '-n', '-c', $command), 0, 'the transport command is POSIX syntax'); + # Keep production guards, apart from the temporary path/uid and the + # explicitly requested short wait or failing command below. + # No test command may access the production runtime directory. + $command =~ s{\Q/run/pve-storage-nvmet\E}{$runtime}g; + $command =~ s/'0:700'/'$ownership'/g; + die "test command escaped temporary runtime\n" if $command =~ m{/run/pve-storage-nvmet}; + if ($short_wait) { + $command =~ s/\bflock -x -w 30 /flock -x -w 0.1 / + or die "test cannot shorten the target lock wait\n"; + } + if ($printf_failure) { + my $prefix = PVE::Tools::shellquote('printf() { return 200; }; '); + $command =~ s{(/bin/sh -c )}{$1$prefix} + or die "test cannot inject the failing printf\n"; + } + if ($sha256_failure) { + my $failure = 'sha256sum() { command sha256sum "$@"; return 200; }; '; + if ($opts{read_owner}) { + $command = $failure . $command; + } else { + my $prefix = PVE::Tools::shellquote($failure); + $command =~ s{(/bin/sh -c )}{$1$prefix} + or die "test cannot inject the failing sha256sum\n"; + } + } + # Kill the whole finite command group if a regression blocks on flock. + return sh('timeout -k 1 2 /bin/sh -c ' . PVE::Tools::shellquote($command)); + }; + for my $option (qw(claim token)) { + for my $invalid ('', 0) { + eval { nv('_nvmet_render_call', [['cat', $output]], $option => $invalid) }; + is( + $@, + "internal error: invalid NVMe target token\n", + "$option '$invalid' is refused", + ); + } + } + for my $case ( + [['cat', $output], read_owner => 1], + [[], read_owner => 1, token => $U{1}], + [[], claim => $U{1}], + [[], claim => $U{1}, expected_owner => 'not-a-digest'], + ) { + my ($steps, %opts) = $case->@*; + eval { nv('_nvmet_render_call', $steps, %opts) }; + like( + $@, + qr/internal error: (?:NVMe owner observation has commands or a token|invalid .*)/, + 'owner metadata is validated before rendering', + ); + } + my ($owner_rc, $owner_out) = $run->([], read_owner => 1); + is($owner_rc, 0, 'an absent owner is observed without the target flock'); + is($owner_out, "missing\n", 'the initial owner snapshot is missing'); + my ($rc) = $run->([], claim => $U{1}, expected_owner => 'missing'); + is($rc, 0, 'the first owner claims'); + my $value = q{literal 'quotes' and $variables}; + ($rc) = $run->([write_step($output, $value)], token => $U{1}); + is($rc, 0, 'the current owner executes a quoted command'); + is(slurp($output), "$value\n", 'outer quoting preserves literal operands'); + { + open(my $held, '+<', "$runtime/lock") or die "open temporary lock: $!\n"; + my $flags = fcntl($held, F_GETFD, 0) // die "get lock FD flags: $!\n"; + fcntl($held, F_SETFD, $flags | FD_CLOEXEC) or die "set lock FD flags: $!\n"; + flock($held, LOCK_EX | LOCK_NB) or die "hold temporary lock: $!\n"; + for my $options ({}, { token => undef, claim => undef }) { + my ($read_rc, $read_out) = $run->([['cat', $output]], $options->%*); + is($read_rc, 0, 'a read completes while another command holds the target flock'); + is($read_out, "$value\n", 'the read returns the actual file content'); + } + ($owner_rc, $owner_out) = $run->([], read_owner => 1); + is($owner_rc, 0, 'an owner snapshot does not wait for a held flock'); + is($owner_out, $digest->("$U{1}\n") . " -\n", 'the snapshot hashes the owner bytes'); + ($rc) = $run->( + [], + claim => $U{2}, + expected_owner => $digest->("$U{1}\n"), + test_short_wait => 1, + ); + is($rc, 200, 'a conflicting claim returns the dedicated busy exit code'); + ($rc) = $run->([['cat', $output]], token => $U{1}, test_short_wait => 1); + is($rc, 1, 'a tokenized call retains the default flock conflict exit code'); + ($rc) = $run->([write_step($output, 'blocked')], token => $U{1}); + is($rc, 124, 'a tokenized call still waits for the exclusive target flock'); + is(slurp("$runtime/owner"), "$U{1}\n", 'the blocked claim did not replace the owner'); + is(slurp($output), "$value\n", 'the blocked change did not run'); + close($held) or die "close temporary lock: $!\n"; + } + my ($read_rc, $read_out) = $run->([['cat', $output]], token => $U{1}); + is($read_rc, 0, 'a tokenized read runs once the exclusive lock is released'); + is($read_out, "$value\n", 'the tokenized read returns the same file content'); + ($rc) = $run->([], claim => $U{2}, expected_owner => $digest->("$U{1}\n")); + is($rc, 0, 'a new owner claims'); + ($rc) = $run->([write_step($output, 'stale')], token => $U{1}); + is($rc, 73, 'an old owner is fenced'); + is(slurp($output), "$value\n", 'a fenced write does not run'); + ($rc) = $run->([['printf', '%s', 'unused']], token => $U{2}, test_printf_failure => 1); + is($rc, 200, 'a tokenized child exit code 200 is passed through'); + is(slurp("$runtime/owner"), "$U{2}\n", 'a failed child did not replace the owner'); + ($rc) = $run->( + [], + claim => $U{3}, + expected_owner => $digest->("$U{2}\n"), + test_printf_failure => 1, + ); + is($rc, 1, 'a failed claim printf is normalized, never reported as busy'); + is(slurp("$runtime/owner"), '', 'the failed printf did not publish a new owner'); + is(slurp($output), "$value\n", 'the failing claim write did no storage work'); + open(my $owner, '>', "$runtime/owner") or die "open restored owner: $!\n"; + print {$owner} "$U{2}\n"; + close($owner) or die "close restored owner: $!\n"; + + ($rc) = $run->( + [], + claim => $U{3}, + expected_owner => $digest->("$U{2}\n"), + test_sha256_failure => 1, + ); + is($rc, 73, 'a failed CAS hash is fenced, never reported as busy'); + is(slurp("$runtime/owner"), "$U{2}\n", 'a failed CAS hash leaves the owner unchanged'); + is(slurp($output), "$value\n", 'a failed CAS hash starts no storage work'); + ($owner_rc, $owner_out) = $run->([], read_owner => 1, test_sha256_failure => 1); + is($owner_rc, 200, 'a failed owner snapshot hash preserves its failure status'); + isnt($owner_out, "missing\n", 'a failed hash is not an absent-owner snapshot'); + is(slurp("$runtime/owner"), "$U{2}\n", 'a failed snapshot leaves the owner unchanged'); + + my $predecessor = $digest->("$U{2}\n"); + my $late_a = nv('_nvmet_render_call', [], claim => $U{4}, expected_owner => $predecessor); + is(system('/bin/sh', '-n', '-c', $late_a), 0, 'the delayed A claim is POSIX syntax'); + $late_a =~ s{\Q/run/pve-storage-nvmet\E}{$runtime}g; + $late_a =~ s/'0:700'/'$ownership'/g; + die "test command escaped temporary runtime\n" if $late_a =~ m{/run/pve-storage-nvmet}; + ($rc) = $run->([], claim => $U{5}, expected_owner => $predecessor); + is($rc, 0, 'B claims after the same existing-owner snapshot'); + my ($late_rc) = sh('timeout -k 1 2 /bin/sh -c ' . PVE::Tools::shellquote($late_a)); + is($late_rc, 73, 'A cannot overwrite B after its stale existing-owner snapshot'); + ($rc) = $run->([], claim => $U{4}, expected_owner => 'missing'); + is($rc, 73, 'a stale missing-owner snapshot cannot overwrite B either'); + is(slurp("$runtime/owner"), "$U{5}\n", 'both stale snapshots preserve the actual owner'); + ($rc) = $run->([write_step($output, 'B')], token => $U{5}); + is($rc, 0, 'B writes after rejecting delayed A'); + is(slurp($output), "B\n", 'B remains the transaction owner'); + + # Any existing owner content, empty included, is observed by its digest + # and can only be claimed with that exact digest. + for my $content ('', "maintenance:1700000000\n") { + open(my $owner, '>', "$runtime/owner") or die "open owner: $!\n"; + print {$owner} $content; + close($owner) or die "close owner: $!\n"; + ($owner_rc, $owner_out) = $run->([], read_owner => 1); + is($owner_rc . $owner_out, '0' . $digest->($content) . " -\n", + 'the owner is observed'); + ($rc) = $run->([], claim => $U{8}, expected_owner => $digest->($content)); + is($rc, 0, 'and can be recovered by its exact digest'); + } + + unlink("$runtime/owner") or die "unlink temporary owner: $!\n"; + ($rc) = $run->([], claim => $U{3}, expected_owner => $digest->("$U{8}\n")); + is($rc, 73, 'a stale digest cannot claim after its owner disappears'); + ok(!-e "$runtime/owner", 'the stale digest does not recreate the missing owner'); + ($rc) = $run->([write_step($output, 'missing')], token => $U{2}); + is($rc, 73, 'a missing marker fails closed'); + + # An unusable directory is refused with its own exit code before the + # flock, by every guarded call. + chmod(0755, $runtime) or die "chmod temporary runtime: $!\n"; + ($rc) = $run->([], claim => $U{3}, expected_owner => 'missing'); + is($rc, 74, 'an unsafe runtime mode refuses a claim'); + ($owner_rc, $owner_out) = $run->([], read_owner => 1); + is($owner_rc . $owner_out, '74', 'and an owner observation, without a snapshot'); + ($rc) = $run->([write_step($output, 'unsafe')], token => $U{2}); + is($rc, 74, 'and a tokenized call, before its token is checked'); + chmod(0700, $runtime) or die "chmod temporary runtime: $!\n"; + rename("$runtime/lock", "$runtime/lock.held") or die "rename temporary lock: $!\n"; + mkdir("$runtime/lock") or die "mkdir temporary lock: $!\n"; + ($rc) = $run->([], claim => $U{3}, expected_owner => 'missing'); + is($rc, 74, 'a lock entry that is not a regular file is refused'); + rmdir("$runtime/lock") or die "rmdir temporary lock: $!\n"; + symlink("$runtime/lock.held", "$runtime/lock") or die "symlink temporary lock: $!\n"; + ($rc) = $run->([], claim => $U{3}, expected_owner => 'missing'); + is($rc, 74, 'so is a symlink lock'); + unlink("$runtime/lock") or die "unlink temporary lock: $!\n"; + rename("$runtime/lock.held", "$runtime/lock") or die "rename temporary lock: $!\n"; + mkdir("$runtime/owner") or die "mkdir temporary owner: $!\n"; + ($rc) = $run->([], claim => $U{3}, expected_owner => 'missing'); + is($rc, 74, 'so is an owner entry that is not a regular file'); + rmdir("$runtime/owner") or die "rmdir temporary owner: $!\n"; + rename($runtime, "$dir/actual") or die "rename temporary runtime: $!\n"; + symlink("$dir/actual", $runtime) or die "symlink temporary runtime: $!\n"; + ($rc) = $run->([], claim => $U{3}, expected_owner => 'missing'); + is($rc, 74, 'and a symlink runtime'); + is(slurp($output), "B\n", 'all refusal paths preserve B output'); +}; + # Last: every command rendered during this test run, and every protocol # violation recorded (by a fake target: a key on a command line or in output, -# a change without the lock; or a local command). +# a change without the lock, a transaction call without a token; or a local +# command). subtest 'every rendered command is a POSIX command chain' => sub { is_deeply(\@VIOLATIONS, [], 'no flow violated the target protocol'); my %shapes; diff --git a/src/test/zfsnvme_test.pm b/src/test/zfsnvme_test.pm index 3b311bfa..8a963bfe 100644 --- a/src/test/zfsnvme_test.pm +++ b/src/test/zfsnvme_test.pm @@ -1053,6 +1053,49 @@ my @ownership = ( eval { $run->(scfg(), [['cat', '/proc/mounts']], op => 'test') }; is($@, "received interrupt\n", 'a stopped task dies with the task marker and nothing else'); } + # The exit codes of the transport guard: 74 (an unusable lock directory) + # names the directory problem for every guarded call, 73 (a failed + # compare-and-set) and 200 (busy) are explained for a claim only, and a + # call without the guard keeps them all as they are. + my $unsafe = + 'unsafe lock directory /run/pve-storage-nvmet on the target: it must be a root-owned' + . ' directory with mode 0700 whose lock and owner entries are regular files'; + for my $option (qw(read_owner claim token)) { + my $steps = $option eq 'token' ? [['cat', '/proc/mounts']] : []; + my %call = (op => 'test', $option => ($option eq 'read_owner' ? 1 : $UUID)); + $call{expected_owner} = 'missing' if $option eq 'claim'; + for my $rc (0, 1, 73, 74, 200, 255) { + local $COMMAND = sub($cmd, %opts) { + die "command 'ssh' failed: exit code $rc\n" if $rc; + return 0; + }; + my $err = $rc ? "exit code $rc" : ''; + $err = $unsafe if $rc == 74; + $err = 'another transaction took over the target' if $option eq 'claim' && $rc == 73; + $err = 'target busy: could not acquire transaction lock' + if $option eq 'claim' && $rc == 200; + is_deeply( + $run->(scfg(), $steps, %call), + { rc => $rc, out => [], err => $err }, + "$option preserves exit $rc, names the directory problem of 74, and only a" + . ' claim classifies 73 as taken over and 200 as busy', + ); + } + local $COMMAND = sub($cmd, %opts) { die "command 'ssh' failed: got timeout\n" }; + is_deeply( + $run->(scfg(), $steps, %call), + { rc => -1, out => [], err => 'timeout' }, + "$option does not confuse an SSH timeout with target busy", + ); + } + for my $rc (73, 74, 200) { + local $COMMAND = sub($cmd, %opts) { die "command 'ssh' failed: exit code $rc\n" }; + is_deeply( + $run->(scfg(), [['cat', '/proc/mounts']], op => 'test'), + { rc => $rc, out => [], err => "exit code $rc" }, + "a call without the guard keeps exit $rc as it is", + ); + } @runs = (); eval { $run->(scfg(), [['cat', '/proc/mounts']]) }; like($@, qr/NVMe target call without label/, 'every call has a label');