Skip to content

Fix error for non slot devices. #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SimonFair wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix error for non slot devices. #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae41552
Fix error for non slot devices.
SimonFair Jun 22, 2026
d0552e0
Update tests/run.php
SimonFair Jun 22, 2026
3fbf5cd
Update tests
SimonFair Jun 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions assets/45d/45drives-disks/assets/index.bbceb330.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23288,6 +23288,21 @@ function zfsAnimation(p5) {
});
return true;
};
p5.showStorageGroupPeers = (cd, animationInfo, diskLocations2, y_offset = 0) => {
const currentDisk = animationInfo?.animation_disks?.[cd];
const group = animationInfo?.animation_groups?.[currentDisk?.group] || [];
if (!currentDisk || group.length < 2) {
return false;
}
group.forEach((dsk) => {
const dsk_idx = diskLocations2.findIndex((loc) => loc.BAY === dsk.name);
if (dsk_idx < 0 || !diskLocations2[dsk_idx]?.image)
return;
const loc = diskLocations2[dsk_idx];
p5.animateZpools(loc.x, loc.y, loc.image.width, loc.image.height + y_offset, p5.zfsAnimationSteps, p5.zfsAnimationIndex, p5.zfsAnimationColors.storage_group.start, p5.zfsAnimationColors.storage_group.end, p5.zfsAnimationDir);
});
Comment on lines +23297 to +23303

@coderabbitai coderabbitai Bot Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Exclude the current disk from peer-group animation.

showStorageGroupPeers currently animates all group members, including cd. For peer-only highlighting, skip the current disk in this loop.

Suggested fix 
     group.forEach((dsk) => {
+      if (dsk.name === cd)
+        return;
       const dsk_idx = diskLocations2.findIndex((loc) => loc.BAY === dsk.name);
       if (dsk_idx < 0 || !diskLocations2[dsk_idx]?.image)
         return;
       const loc = diskLocations2[dsk_idx];
       p5.animateZpools(loc.x, loc.y, loc.image.width, loc.image.height + y_offset, p5.zfsAnimationSteps, p5.zfsAnimationIndex, p5.zfsAnimationColors.storage_group.start, p5.zfsAnimationColors.storage_group.end, p5.zfsAnimationDir);
     });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
group.forEach((dsk) => {
const dsk_idx = diskLocations2.findIndex((loc) => loc.BAY === dsk.name);
if (dsk_idx < 0 || !diskLocations2[dsk_idx]?.image)
return;
const loc = diskLocations2[dsk_idx];
p5.animateZpools(loc.x, loc.y, loc.image.width, loc.image.height + y_offset, p5.zfsAnimationSteps, p5.zfsAnimationIndex, p5.zfsAnimationColors.storage_group.start, p5.zfsAnimationColors.storage_group.end, p5.zfsAnimationDir);
});
group.forEach((dsk) => {
if (dsk.name === cd)
return;
const dsk_idx = diskLocations2.findIndex((loc) => loc.BAY === dsk.name);
if (dsk_idx < 0 || !diskLocations2[dsk_idx]?.image)
return;
const loc = diskLocations2[dsk_idx];
p5.animateZpools(loc.x, loc.y, loc.image.width, loc.image.height + y_offset, p5.zfsAnimationSteps, p5.zfsAnimationIndex, p5.zfsAnimationColors.storage_group.start, p5.zfsAnimationColors.storage_group.end, p5.zfsAnimationDir);
});
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@assets/45d/45drives-disks/assets/index.bbceb330.js` around lines 23297 -
23303, The forEach loop that animates storage group members currently includes
the current disk in the animation. Within the group.forEach loop where dsk is
iterated, add an additional condition to skip the current disk (cd) so that only
peer members are animated. Check if dsk.name (or dsk) matches the current disk
identifier and return early if it does, before calling p5.animateZpools. This
ensures peer-only highlighting by excluding the current disk from the animation
loop.

return true;
};
p5.showAnimations = (cd, zfsInfo, diskLocations2, y_offset = 0) => {
if (zfsInfo.zfs_installed) {
if (Array.isArray(zfsInfo.zpools) && zfsInfo.zfs_disks && zfsInfo.zfs_disks[cd]) {
Expand Down Expand Up @@ -23338,6 +23353,7 @@ function zfsAnimation(p5) {
}
});
});
p5.showStorageGroupPeers(cd, zfsInfo, diskLocations2, y_offset);
return true;
}
}
Expand Down
64 changes: 17 additions & 47 deletions php/zfs_info.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,17 @@ function zfs_drivemap_lookup()
$lookup[$value] = $bay_id;
$lookup[basename($value)] = $bay_id;
}

$storage_label = $slot['storage-label'] ?? '';
if (is_string($storage_label) && preg_match('/^disk([0-9]+)$/', $storage_label, $match)) {
$lookup[$storage_label] = $bay_id;
$lookup['md' . $match[1]] = $bay_id;
$lookup['md' . $match[1] . 'p1'] = $bay_id;
$lookup['/dev/md' . $match[1]] = $bay_id;
$lookup['/dev/md' . $match[1] . 'p1'] = $bay_id;
$lookup['/dev/mapper/md' . $match[1]] = $bay_id;
$lookup['/dev/mapper/md' . $match[1] . 'p1'] = $bay_id;
}
}
}

Expand Down Expand Up @@ -401,54 +412,10 @@ function zpool_status_parse($status_obj, $key, $pool_name, $disk_lookup = [])

function verify_zfs_device_format($status_obj, $pool_name, $disk_lookup = [])
{
// Frontend drive-to-bay mapping relies on "card-drive" aliases (e.g. 1-1).
// Emit warnings when pool members do not follow that convention.
// Non-bay devices such as boot, flash, and standalone NVMe pools are valid
// ZFS members, but there is no slot to annotate in the drivemap UI.
// Keep them in the pool list and skip user-facing warnings.
$alert = [];
if (!isset($status_obj[$pool_name])) {
return $alert;
}

$default_pattern = '/^\t (\d+-\d+)(?:-part[0-9])?\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+).*/m';
$unsupported_pattern = '/^\t (\S+)(?:-part[0-9])?\s+(\S+)\s+(\S+)\s+(\S+)\s+(\S+).*/m';

preg_match_all($default_pattern, $status_obj[$pool_name], $default_matches, PREG_SET_ORDER);
preg_match_all($unsupported_pattern, $status_obj[$pool_name], $unsupported_matches, PREG_SET_ORDER);

$default_disks = [];
foreach ($default_matches as $match) {
$default_disks[] = $match[1];
}

$unsupported_disks = [];
foreach ($unsupported_matches as $match) {
$unsupported_disks[] = $match[1];
}

if (count($unsupported_disks) > count($default_disks)) {
$filtered = array_values(array_diff($unsupported_disks, $default_disks));
$filtered = array_values(array_filter($filtered, function ($name) {
return !preg_match('/^(\d+-\d+)(?:-part[0-9])/', $name);
}));
$filtered = array_values(array_filter($filtered, function ($name) use ($disk_lookup) {
return !preg_match('/^\d+-\d+$/', canonical_zfs_disk_name($name, $disk_lookup));
}));

if (!$filtered) {
return $alert;
}

$alert[] = "ZFS status displayed by this module for zpool '$pool_name' may be incomplete.\n\n";
$alert[] = "This module can only display zfs status information for devices that are created using a device alias.\n\n";
$alert[] = "This can be done using the 45Drives cockpit-zfs-manager package:\nhttps://github.com/45Drives/cockpit-zfs-manager/releases/\n\n";
if ($filtered) {
$alert[] = "The following zfs devices do not conform:\n";
foreach ($filtered as $disk) {
$alert[] = "\t $disk\n";
}
}
$alert[] = "\n";
}

return $alert;
Comment on lines +415 to 419

@coderabbitai coderabbitai Bot Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

verify_zfs_device_format() now suppresses all warnings, not only non-slot ones.

Line 415-419 returns an empty array unconditionally, so genuinely malformed or unexpectedly mapped members also lose diagnostics. That widens behavior beyond the stated non-slot suppression and weakens the API’s warning signal.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@php/zfs_info.php` around lines 415 - 419, The verify_zfs_device_format()
function currently returns an empty alert array unconditionally, which
suppresses warnings for all cases including genuinely malformed or unexpectedly
mapped devices. Modify the return statement to conditionally return an empty
array only for valid non-bay devices (boot, flash, standalone NVMe pools), while
ensuring that genuinely malformed or unexpectedly mapped members still populate
and return the alert array with appropriate diagnostic warnings. This requires
adding conditional logic before the return statement to distinguish between
non-slot devices that should be silenced and actual format violations that
should still be reported.

}

Expand Down Expand Up @@ -624,6 +591,9 @@ function generate_zfs_info()
}
foreach ($pool['vdevs'] as $vdev) {
foreach ($vdev['disks'] as $disk) {
if (!isset($disk['name']) || !preg_match('/^\d+-\d+$/', $disk['name'])) {
continue;
}
$disk_entries[$disk['name']] = [
'zpool_name' => $pool['name'],
'zpool_used' => $pool['used'] ?? '-',
Expand Down
2 changes: 2 additions & 0 deletions tests/fixtures/zfs_array/zfs_list.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
disk1 10G 90G 10G /mnt/disk1
zmirror 20G 180G 20G /mnt/zmirror
5 changes: 5 additions & 0 deletions tests/fixtures/zfs_array/zpool_iostat_disk1.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
capacity operations bandwidth
pool alloc free read write read write
------------------------------------- ----- ----- ----- ----- ----- -----
disk1 10G 90G 1 2 10M 20M
/dev/mapper/md1p1 10G 90G 1 2 10M 20M
5 changes: 5 additions & 0 deletions tests/fixtures/zfs_array/zpool_iostat_path_disk1.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
capacity operations bandwidth
pool alloc free read write read write
------------------------------------- ----- ----- ----- ----- ----- -----
disk1 10G 90G 1 2 10M 20M
/dev/mapper/md1p1 10G 90G 1 2 10M 20M
7 changes: 7 additions & 0 deletions tests/fixtures/zfs_array/zpool_iostat_path_zmirror.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
capacity operations bandwidth
pool alloc free read write read write
------------------------------------- ----- ----- ----- ----- ----- -----
zmirror 20G 180G 1 2 10M 20M
mirror-0 20G 180G 1 2 10M 20M
/dev/nvme1n1p1 10G 90G 1 2 5M 10M
/dev/nvme2n1p1 10G 90G 1 2 5M 10M
7 changes: 7 additions & 0 deletions tests/fixtures/zfs_array/zpool_iostat_zmirror.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
capacity operations bandwidth
pool alloc free read write read write
------------------------------------- ----- ----- ----- ----- ----- -----
zmirror 20G 180G 1 2 10M 20M
mirror-0 20G 180G 1 2 10M 20M
nvme1n1p1 10G 90G 1 2 5M 10M
nvme2n1p1 10G 90G 1 2 5M 10M
2 changes: 2 additions & 0 deletions tests/fixtures/zfs_array/zpool_list.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
disk1 100G 10G 90G - - 10% 10% 1.00x ONLINE -
zmirror 200G 20G 180G - - 10% 10% 1.00x ONLINE -
9 changes: 9 additions & 0 deletions tests/fixtures/zfs_array/zpool_status_disk1.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
pool: disk1
state: ONLINE
config:

NAME STATE READ WRITE CKSUM
disk1 ONLINE 0 0 0
/dev/mapper/md1p1 ONLINE 0 0 0

errors: No known data errors
9 changes: 9 additions & 0 deletions tests/fixtures/zfs_array/zpool_status_path_disk1.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
pool: disk1
state: ONLINE
config:

NAME STATE READ WRITE CKSUM
disk1 ONLINE 0 0 0
/dev/mapper/md1p1 ONLINE 0 0 0

errors: No known data errors
11 changes: 11 additions & 0 deletions tests/fixtures/zfs_array/zpool_status_path_zmirror.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
pool: zmirror
state: ONLINE
config:

NAME STATE READ WRITE CKSUM
zmirror ONLINE 0 0 0
mirror-0 ONLINE 0 0 0
/dev/nvme1n1p1 ONLINE 0 0 0
/dev/nvme2n1p1 ONLINE 0 0 0

errors: No known data errors
11 changes: 11 additions & 0 deletions tests/fixtures/zfs_array/zpool_status_zmirror.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
pool: zmirror
state: ONLINE
config:

NAME STATE READ WRITE CKSUM
zmirror ONLINE 0 0 0
mirror-0 ONLINE 0 0 0
nvme1n1p1 ONLINE 0 0 0
nvme2n1p1 ONLINE 0 0 0

errors: No known data errors
27 changes: 27 additions & 0 deletions tests/run.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,34 @@ function run_ported_dmap_alias_lines($root, $ctx, $case_name)
assert_true(!isset($zfs_raw_info['zfs_disks']['sda1']), 'raw-device zfs_info does not expose raw sda1 key');
assert_true(empty($zfs_raw_info['warnings']), 'raw-device zfs_info suppresses alias warning when drivemap can resolve devices');

$original_zfs_fixture_dir = getenv('DRIVEMAP_ZFS_FIXTURE_DIR');
putenv('DRIVEMAP_ZFS_FIXTURE_DIR=' . $fixtures . '/zfs_array');
try {
[$zfs_array_code, $zfs_array_body] = run_api_action($root, 'zfs_info');
assert_equal($zfs_array_code, 0, 'zfs_info array-device endpoint exits successfully');
$zfs_array_info = json_decode($zfs_array_body, true);
assert_true(is_array($zfs_array_info), 'zfs_info array-device endpoint returns JSON');
assert_true(isset($zfs_array_info['zfs_disks']['1-1']), 'array-device zfs_info maps md1p1 pool to disk1 bay');
assert_equal($zfs_array_info['zfs_disks']['1-1']['zpool_name'] ?? '', 'disk1', 'array-device zfs_info reports disk1 pool on bay 1-1');
assert_true(!isset($zfs_array_info['zfs_disks']['/dev/mapper/md1p1']), 'array-device zfs_info does not expose md mapper key');
assert_true(!isset($zfs_array_info['zfs_disks']['nvme1n1p1']), 'array-device zfs_info ignores unmapped nvme pool members');
assert_true(!isset($zfs_array_info['zfs_disks']['nvme2n1p1']), 'array-device zfs_info ignores second unmapped nvme pool member');
assert_true(empty($zfs_array_info['warnings']), 'array-device zfs_info suppresses warning for non-slot zfs devices');
} finally {
if ($original_zfs_fixture_dir === false) {
putenv('DRIVEMAP_ZFS_FIXTURE_DIR');
} else {
putenv('DRIVEMAP_ZFS_FIXTURE_DIR=' . $original_zfs_fixture_dir);
}
}

require_once $root . '/php/zfs_info.php';
$malformed_alerts = verify_zfs_device_format([
'tank' => "\ttank ONLINE 0 0 0\n\t mirror-0 ONLINE 0 0 0\n\t bad-disk ONLINE 0 0 0\n",
], 'tank', []);
assert_true(!empty($malformed_alerts), 'zfs device format warns for unresolved malformed members');
assert_true(strpos(implode('', $malformed_alerts), 'bad-disk') !== false, 'zfs device format warning names malformed member');
Comment on lines +581 to +582

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Update stale warning assertions to match current verify_zfs_device_format behavior.

These expectations now conflict with the implementation that returns no warnings, so this test will fail after the warning-removal change.

Proposed fix 
-$malformed_alerts = verify_zfs_device_format([
-  'tank' => "\ttank        ONLINE       0     0     0\n\t  mirror-0  ONLINE       0     0     0\n\t    bad-disk  ONLINE       0     0     0\n",
-], 'tank', []);
-assert_true(!empty($malformed_alerts), 'zfs device format warns for unresolved malformed members');
-assert_true(strpos(implode('', $malformed_alerts), 'bad-disk') !== false, 'zfs device format warning names malformed member');
+$malformed_alerts = verify_zfs_device_format([
+  'tank' => "\ttank        ONLINE       0     0     0\n\t  mirror-0  ONLINE       0     0     0\n\t    bad-disk  ONLINE       0     0     0\n",
+], 'tank', []);
+assert_true(empty($malformed_alerts), 'zfs device format suppresses warnings for unresolved malformed members');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
assert_true(!empty($malformed_alerts), 'zfs device format warns for unresolved malformed members');
assert_true(strpos(implode('', $malformed_alerts), 'bad-disk') !== false, 'zfs device format warning names malformed member');
$malformed_alerts = verify_zfs_device_format([
'tank' => "\ttank ONLINE 0 0 0\n\t mirror-0 ONLINE 0 0 0\n\t bad-disk ONLINE 0 0 0\n",
], 'tank', []);
assert_true(empty($malformed_alerts), 'zfs device format suppresses warnings for unresolved malformed members');
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/run.php` around lines 581 - 582, The assertions at lines 581-582 are
testing for the presence of malformed device warnings (checking that
$malformed_alerts is not empty and contains 'bad-disk'), but the current
implementation of verify_zfs_device_format no longer generates these warnings.
Update these assertions to match the current behavior by changing them to assert
that $malformed_alerts is empty, which reflects that no warnings should be
returned when malformed members are encountered.


$lookup_refresh_path = $ctx['out_dir'] . '/zfs_lookup_refresh.json';
file_put_contents($lookup_refresh_path, json_encode([
'rows' => [[
Expand Down