Skip to content

Fix premature recovery from DiskFull read-only mode - #19126

Open
d-wang-commit wants to merge 1 commit into
apache:masterfrom
d-wang-commit:fix-diskfull-auto-recovery
Open

d-wang-commit wants to merge 1 commit into
apache:masterfrom
d-wang-commit:fix-diskfull-auto-recovery

Conversation

@d-wang-commit

Copy link
Copy Markdown
Contributor

Description

Keep DiskFull recovery consistent with directory protection

A data or snapshot filesystem can remain full while the aggregate free-space ratio exceeds the warning threshold. Directory selection then enters ReadOnly(DiskFull), but the next heartbeat restores Running using only the aggregate ratio. Repeated operations can keep switching the node between these states without any space being freed.

Record a recovery check for each failed directory source in CommonConfig. The heartbeat restores Running only when the aggregate check passes and every recorded source has an eligible directory with space. Constructor failures use an immutable folder list as the key; live FolderManager instances use their identity and retain their folder-health restrictions. Repeated reports for the same key replace the callback. Leaving read-only mode clears the recorded checks.

Filesystem checks run outside the status lock. Recovery rechecks the exact ReadOnly(DiskFull) state before changing it, preserving manual, stopping, and unrecoverable-error transitions. The existing IOException retry and unrecoverable-error handling remain unchanged.

Tests

  • UT: extend CommonConfigTest for multiple failed sources, callback replacement, concurrent higher-priority reasons, and manual reset.
  • UT: extend DataNodeInternalRPCServiceImplDiskTest for healthy aggregate metrics with blocked sources, and recovered sources with zero or insufficient aggregate space.
  • UT: add FolderManagerDiskFullRecoveryTest for repeated snapshot initialization failures, multiple directory groups, unhealthy candidates, and callers that opt out of changing node status. Run the existing FolderManagerTest as regression coverage.
  • Validation after rebasing onto upstream/master:
    • Full-reactor mvn -B -ntp -o test -Dtest=CommonConfigTest,FolderManagerTest,FolderManagerDiskFullRecoveryTest,DataNodeInternalRPCServiceImplDiskTest -Dsurefire.failIfNoSpecifiedTests=false: passed; 54 tests, 0 failures/errors, 4 pre-existing skips.
    • Full-reactor mvn -B -ntp -o test-compile -DskipTests -P with-zh-locale: passed.
    • Spotless formatting and git diff --check: passed.
  • IT: no integration test was added or run. Disk capacity is simulated in unit tests; a real ENOSPC cluster test was not run.

Side effects and risks

Recovery now probes previously failed directories using the existing hasSpace behavior, including its retries, so a heartbeat may spend longer checking a full filesystem. Callback references are retained until the node leaves read-only mode, with one entry per distinct key. Folder health uses a concurrent map because recovery reads it from the heartbeat thread.

Space checks are snapshots. A new disk-full report racing with the final recovery step can still require the next business operation to trigger protection again; this change addresses repeated recovery while already-recorded sources remain full.


This PR has:

  • been self-reviewed.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage.

Key changed/added classes (or packages if there are too many classes) in this PR
  • CommonConfig: records failed sources and coordinates guarded disk recovery.
  • DirectoryStrategy, FolderManager, DataRegion: register the recovery condition at directory-related DiskFull entries.
  • DataNodeInternalRPCServiceImpl: uses the shared recovery method after the aggregate disk check passes.
  • CommonConfigTest, DataNodeInternalRPCServiceImplDiskTest, FolderManagerDiskFullRecoveryTest: cover state transitions, heartbeat gates, and directory failures.

Require affected directories to recover before restoring Running status.
Preserve manual and unrecoverable read-only reasons.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant