Improving mount diagnostics by adamoutler · Pull Request #1364 · netalertx/NetAlertX

adamoutler · 2025-12-22T02:15:06Z

This commit improves mount diagnostics

adds an R column and renames "writable" to W. R/W are tested in two separate columns
Plain text description of failure is added to message below the table to support fonts not capable of emoji ✅| ❌
Add new unit tests for read issues
Adjust old unit tests to handle RW table format

Summary by CodeRabbit

Release Notes

New Features
- Added read permission diagnostics to mount health checks with detection of read permission errors
- New Read column in diagnostic table output displays mount readability status
- Extended diagnostic warnings to report read permission issues alongside write errors
Tests
- Introduced new test scenarios covering unreadable mount configurations

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-22T02:15:16Z

Walkthrough

This change extends mount diagnostics to track and report read permissions alongside write permissions. It adds readability checks to MountCheckResult, updates the diagnostic table UI to display a Read column, introduces test configurations for readonly mount scenarios, and enhances test validation to parse and verify readable state in diagnostic output.

Changes

Cohort / File(s)	Summary
Core Diagnostic Logic `install/production-filesystem/entrypoint.d/10-mounts.py`	Added `is_readable` and `read_error` fields to `MountCheckResult`. Introduced `_resolve_readable_state()` helper to determine readability by checking first existing parent directory. Extended `analyze_path()` to compute readability and set read errors. Updated health checks, warning messages, and exit conditions to account for read errors alongside write errors. Added Read column to diagnostic table UI with corresponding symbols and formatting.
Docker Test Configurations `test/docker_tests/configurations/mount-tests/docker-compose.mount-test.*.yml` (api_noread, data_noread, db_noread, tmp_noread)	Added four new docker-compose YAML configurations for mount-test scenarios. Each defines a netalertx service with host networking, restricted capabilities, environment variables, data volumes, and tmpfs mounts. Configurations support testing writable-but-unreadable mount paths via external permission setup.
Test Infrastructure Updates `test/docker_tests/conftest.py`	Minor formatting adjustment; added blank line after image assignment.
Container Environment Tests `test/docker_tests/test_container_environment.py`	Enhanced mount table parsing to handle both legacy (5-column) and current (6-column) formats. Updated error messages and added dynamic column detection. Expanded assertions to map both formats to consistent internal representation. Added "data" to key sets for mounting scenario alignment.
Mount Diagnostics Test Suite `test/docker_tests/test_mount_diagnostics_pytest.py`	Added `readable` field to `MountTableRow` dataclass. Updated `parse_mount_table()` to expect 7 columns (including readable as first column). Extended `assert_table_row()` signature to validate readable state. Added runtime noread permission setup via docker exec. Introduced four new test scenarios (data_noread, db_noread, tmp_noread, api_noread) to verify read permission diagnostics. Enhanced test image fixture with timeout and Docker availability handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Areas requiring additional attention:

install/production-filesystem/entrypoint.d/10-mounts.py: Review _resolve_readable_state() logic for edge cases when traversing parent directories; verify read-error handling mirrors write-error logic consistently
test/docker_tests/test_mount_diagnostics_pytest.py: Verify column index mappings in parse_mount_table() and assert_table_row() handle both old and new table formats correctly; review runtime permission setup via docker exec for robustness
Cross-file validation: Ensure diagnostic table output format changes align between core logic and test expectations

Poem

🐰 A rabbit checks what files we can read,
From parent paths where access is freed,
Now warnings show read errors with pride,
Readable, writable, side by side,
Permissions tracked with diagnostic glide! 📋✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: adding read diagnostics capabilities to mount diagnostics via new R column, read_error tracking, and related test updates.
Docstring Coverage	✅ Passed	Docstring coverage is 80.95% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1f355ad and 95e9315.

⛔ Files ignored due to path filters (1)

test/docker_tests/configurations/test_results.log is excluded by !**/*.log

📒 Files selected for processing (8)

install/production-filesystem/entrypoint.d/10-mounts.py
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.api_noread.yml
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.data_noread.yml
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.db_noread.yml
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.tmp_noread.yml
test/docker_tests/conftest.py
test/docker_tests/test_container_environment.py
test/docker_tests/test_mount_diagnostics_pytest.py

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.py: Use mylog(level, [message]) for logging; levels are: none/minimal/verbose/debug/trace. Use none for most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always use get_setting_value() to retrieve configuration values.
Use environment variables (NETALERTX_DB, NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like /data/db or relative paths.
Use helper.py functions (timeNowDB, normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings via ccd() in server/initialise.py or per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.

Files:

install/production-filesystem/entrypoint.d/10-mounts.py
test/docker_tests/test_mount_diagnostics_pytest.py
test/docker_tests/test_container_environment.py
test/docker_tests/conftest.py

🧠 Learnings (5)

📚 Learning: 2025-10-26T17:09:18.621Z

Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: .devcontainer/scripts/setup.sh:146-148
Timestamp: 2025-10-26T17:09:18.621Z
Learning: In `.devcontainer/scripts/setup.sh` and other devcontainer setup scripts for NetAlertX, chmod 666 on /var/run/docker.sock is acceptable because devcontainer environments are single-user development contexts where convenience can take priority over strict permission hardening.

Applied to files:

test/docker_tests/configurations/mount-tests/docker-compose.mount-test.tmp_noread.yml
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.db_noread.yml
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.data_noread.yml
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.api_noread.yml

📚 Learning: 2025-09-20T14:09:29.159Z

Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.

Applied to files:

test/docker_tests/configurations/mount-tests/docker-compose.mount-test.tmp_noread.yml
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.db_noread.yml
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.data_noread.yml
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.api_noread.yml
test/docker_tests/test_container_environment.py

📚 Learning: 2025-09-20T03:01:19.912Z

Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:18-19
Timestamp: 2025-09-20T03:01:19.912Z
Learning: In the NetAlertX repository, .devcontainer/Dockerfile is auto-generated and should not be reviewed directly. Review comments about dependencies and build steps should be directed at the root Dockerfile where the actual source commands are located.

Applied to files:

test/docker_tests/configurations/mount-tests/docker-compose.mount-test.db_noread.yml
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.data_noread.yml

📚 Learning: 2025-10-19T01:40:57.095Z

Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1230
File: .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template:1-2
Timestamp: 2025-10-19T01:40:57.095Z
Learning: In the NetAlertX repository, .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template is an auto-generated file that is intentionally committed to source control. It cannot be regenerated automatically outside the devcontainer environment and is required for the devcontainer to start, creating a bootstrap dependency.

Applied to files:

test/docker_tests/configurations/mount-tests/docker-compose.mount-test.db_noread.yml
test/docker_tests/configurations/mount-tests/docker-compose.mount-test.data_noread.yml

📚 Learning: 2025-12-08T01:59:00.903Z

Learnt from: CR
Repo: jokob-sk/NetAlertX PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-08T01:59:00.903Z
Learning: Applies to **/*.py : Use environment variables (`NETALERTX_DB`, `NETALERTX_LOG`, etc.) everywhere instead of hardcoding paths like `/data/db` or relative paths.

Applied to files:

test/docker_tests/test_container_environment.py

🪛 Ruff (0.14.8)

test/docker_tests/test_mount_diagnostics_pytest.py

220-220: subprocess call: check for execution of untrusted input

(S603)

221-221: Starting a process with a partial executable path

(S607)

339-339: Probable insecure usage of temporary file or directory: "/tmp"

(S108)

569-569: subprocess call: check for execution of untrusted input

(S603)

570-570: Starting a process with a partial executable path

(S607)

653-653: subprocess call: check for execution of untrusted input

(S603)

682-682: subprocess call: check for execution of untrusted input

(S603)

750-750: String contains ambiguous ➖ (HEAVY MINUS SIGN). Did you mean - (HYPHEN-MINUS)?

(RUF001)

750-750: String contains ambiguous ➖ (HEAVY MINUS SIGN). Did you mean - (HYPHEN-MINUS)?

(RUF001)

test/docker_tests/test_container_environment.py

664-666: Avoid specifying long messages outside the exception class

(TRY003)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: docker-tests
GitHub Check: docker_dev

🔇 Additional comments (25)

test/docker_tests/configurations/mount-tests/docker-compose.mount-test.api_noread.yml (1)

1-39: LGTM!

The docker-compose configuration correctly sets up the api_noread test scenario with appropriate capability restrictions, environment variables, and mount configurations. The clear documentation comment at the top explaining the expected outcome (R=❌, W=✅ for /tmp/api) is helpful for understanding the test intent.

test/docker_tests/conftest.py (1)

20-22: LGTM!

Minor formatting change with no functional impact.

test/docker_tests/configurations/mount-tests/docker-compose.mount-test.tmp_noread.yml (1)

1-39: LGTM!

The docker-compose configuration correctly establishes the tmp_noread test scenario with consistent structure matching other noread test configurations.

test/docker_tests/configurations/mount-tests/docker-compose.mount-test.db_noread.yml (1)

1-39: LGTM!

The docker-compose configuration correctly sets up the db_noread test scenario, following the established pattern used by other noread test configurations.

test/docker_tests/test_container_environment.py (3)

592-621: Well-structured dual-format parsing.

The logic cleanly handles both legacy (5-column with "Writeable") and current (6-column with "R" and "W") table formats. The detection based on header content ("| R" in line and "| W" in line) is robust.

641-684: Clean refactor with unified column handling.

The label-to-value mapping elegantly supports both legacy and current formats while providing clear error messages for debugging. The static analysis hint (TRY003) about the long message in the exception is a minor style preference; detailed assertion messages are valuable in test code for diagnosing failures.

1006-1016: LGTM!

The addition of "data" to the key set ensures proper mount point validation for all required directories in the test setup.

test/docker_tests/configurations/mount-tests/docker-compose.mount-test.data_noread.yml (1)

1-39: LGTM!

The docker-compose configuration correctly establishes the data_noread test scenario, maintaining consistency with other noread test configurations in the suite.

install/production-filesystem/entrypoint.d/10-mounts.py (7)

27-48: LGTM - Well-designed dataclass extension.

The addition of is_readable and read_error fields to MountCheckResult cleanly mirrors the existing is_writeable and write_error pattern, maintaining consistency in the data model.

124-143: LGTM - Consistent pattern with writeable check.

The _resolve_readable_state function follows the same parent-ascending pattern as _resolve_writeable_state. Note that unlike the write check, this doesn't perform an actual read test (analogous to the OverlayFS write verification) - this is likely acceptable since read operations don't have the same copy-up concerns on OverlayFS.

168-181: Consistent integration of readability checks.

The readability check follows the same pattern as the existing writability check, correctly setting read_error only for non-secondary roles.

232-265: Improved diagnostic output with per-path issue enumeration.

The refactored print_warning_message now provides actionable, path-specific issue details rather than a generic warning. This aligns well with the PR objective of improving mount diagnostics.

274-291: Correct integration of read_error into health checks.

The _sub_result_is_healthy function now properly considers read_error alongside write_error for both persist and ramdisk categories.

375-459: Well-formatted diagnostic table with new R column.

The table formatting correctly integrates the new Read (R) column alongside the renamed Write (W) column, maintaining visual consistency with the existing diagnostic output.

466-468: Verify exit condition behavior change is intentional.

The exit condition now triggers only on read/write permission errors (has_rw_errors), rather than including dataloss_risk or performance_issue. This means containers with data loss risk will display warnings but continue startup. Please verify this behavioral change aligns with the intended user experience.

test/docker_tests/test_mount_diagnostics_pytest.py (10)

53-63: LGTM!

The addition of readable: bool is consistent with other non-optional fields (writeable, mount, dataloss) that always have a determinable True/False state, while ramdisk and performance remain Optional[bool] since they can be "not applicable" (➖).

107-137: LGTM!

The column count check and index mappings are correctly updated to accommodate the new readable column at position 1, with all subsequent columns shifted appropriately.

145-194: LGTM!

The function signature and column index updates are consistent. The readable parameter follows the existing Expectation = UNSET pattern, and all _check calls have correct column indices after the shift.

219-235: Well-implemented Docker CLI resilience.

The fixture now properly handles edge cases:

Explicit timeout=10 per coding guidelines

FileNotFoundError when Docker CLI is missing

TimeoutExpired for unresponsive Docker daemon

Non-zero return codes with helpful error messages

This prevents cryptic test failures in CI environments where Docker may be unavailable.

314-355: LGTM!

The noread test scenarios are well-structured:

Clear separation between persistent (/data/*) and transient (/tmp/*) paths

expected_exit_code=0 is correct since the container doesn't exit early for permission issues; the diagnostic tool exit code is handled separately at line 711-712

Good coverage of different mount configurations for readability testing

567-577: Good defensive cleanup.

Explicitly removing stale containers by name prevents test flakiness when previous runs failed to clean up properly. The check=False and timeout=30 are appropriate for this cleanup step.

632-693: Well-designed permission setup with verification.

The noread preparation logic is thorough:

chmod 0300 correctly removes read permission while preserving write+execute

Baseline setup (lines 646-647) ensures stable diagnostics before modifying target

The Python-based verification (lines 663-692) confirms the effective permissions as the app user

Running as --user netalertx ensures permissions are tested from the application's perspective

The verification pattern (not r and w and x) is the correct check for the intended -wx state.

407-434: LGTM!

The path categorization and assertions are correct:

Paths starting with /data use persistent-mount assertions (ramdisk=None, performance=None)

Other paths use ramdisk assertions (ramdisk=True, performance=True)

dataloss=True is appropriate for noread scenarios since data integrity cannot be verified without read access

The early return at line 434 correctly skips the remaining scenario-specific checks after handling noread cases.

744-778: LGTM!

The test data and assertions are correctly updated:

Sample output reflects the new 7-column format with separate R and W columns

Both table rows show ✅ for R, matching the readable=True assertions

Existing column assertions remain unchanged

708-714: LGTM!

The exit code logic correctly extends to handle noread scenarios. The diagnostic tool returns exit code 1 for permission issues (both read and write), which is consistent behavior. The special case for active_config_unwritable (exit 0 with warning only) is preserved.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

adamoutler · 2025-12-22T02:28:52Z

New table format:

Startup pre-checks
netalertx-test-mount-api_ramdisk  | --> storage permission.sh 
netalertx-test-mount-api_ramdisk  | --> data migration.sh 
netalertx-test-mount-api_ramdisk  | --> mounts.py 
netalertx-test-mount-api_ramdisk  |  Path                     | R | W | Mount | RAMDisk | Performance | DataLoss 
netalertx-test-mount-api_ramdisk  | --------------------------+---+---+-------+---------+-------------+----------
netalertx-test-mount-api_ramdisk  |  /data                    | ✅| ✅|   ✅  |    ➖   |      ➖     |    ✅     
netalertx-test-mount-api_ramdisk  |  /data/db                 | ✅| ✅|   ✅  |    ➖   |      ➖     |    ✅     
netalertx-test-mount-api_ramdisk  |  /data/config             | ✅| ✅|   ✅  |    ➖   |      ➖     |    ✅     
netalertx-test-mount-api_ramdisk  |  /tmp/run/tmp             | ✅| ✅|   ✅  |    ✅   |      ✅     |    ✅     
netalertx-test-mount-api_ramdisk  |  /tmp/api                 | ✅| ✅|   ✅  |    ✅   |      ✅     |    ✅     
netalertx-test-mount-api_ramdisk  |  /tmp/log                 | ✅| ✅|   ✅  |    ✅   |      ✅     |    ✅     
netalertx-test-mount-api_ramdisk  |  /tmp/run                 | ✅| ✅|   ✅  |    ✅   |      ✅     |    ✅     
netalertx-test-mount-api_ramdisk  |  /tmp/nginx/active-config | ✅| ✅|   ✅  |    ✅   |      ✅     |    ✅

Improving mount diagnostics

95e9315

jokob-sk merged commit 935453a into netalertx:main Dec 22, 2025
6 checks passed

coderabbitai bot mentioned this pull request Jan 4, 2026

Feature: Passive PUID/PGID Support & Startup Sequence Refactor #1381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improving mount diagnostics#1364

Improving mount diagnostics#1364
jokob-sk merged 1 commit intonetalertx:mainfrom
adamoutler:improve-mount-built-in-test

adamoutler commented Dec 22, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 22, 2025 •

edited

Loading

Uh oh!

adamoutler commented Dec 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

adamoutler commented Dec 22, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai bot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

adamoutler commented Dec 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adamoutler commented Dec 22, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 22, 2025 •

edited

Loading