HDDS-13173. Clean up temporary checkpoint directories on snapshot defrag failure

[cchung100m]

What changes were proposed in this pull request?

Root Cause: When snapshot defragmentation fails due to transient errors, the temporary checkpoint directories are not cleaned up. Over 3+ days of continuous operations, these orphaned directories accumulate and exhaust disk space, causing OM startup failures.

Solution: Implement multi-layer exception handling with an explicit defragSuccessful flag.

1. Add success flag defragSuccessful: Initialize to false, set to true only after atomicSwitchSnampshotDB() succeeds.
2. Protect close(): Wrap in try-catch to prevent cleanup bypass
3. Protect old checkpoint deletion: Wrap in try-catch but continue with flag set
4. Add cleanup in finally block: Delete checkpoint directory if !defragSuccessful
5. Add metrics: Track cleanup failures via incNumSnapshotDefragFails()

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13173

How was this patch tested?

• Existing unit and integration tests validate the current change cause unexpected impacts
• Add new test case testCheckpointCleanupOnDefragFailure to validate the exception handling

[adoroszlai]

Thanks @cchung100m for working on this.

• Stack trace in the Jira issue indicates root cause of OM startup failure was No space left on device. Can you please explain how a change in SnapshotDefragService solves that problem?
• I'm not sure allowing OM to start with No space left on device is useful/desired.
• Please describe testing in more detail. "Existing tests" does not cut it, since this is supposed to fix a bug, but existing tests are already passing without the patch.

[cchung100m]

Hi @adoroszlai

Sorry for the late reply and the incomplete PR before. I think the No space left on device is caused by checkpoint directory accumulation. Therefore, I added cleanup code that deletes the checkpoint directory on failure. I implemented multi-layer exception handling with an explicit defragSuccessful flag detailed in the PR description.

[adoroszlai]

I think the No space left on device is caused by checkpoint directory accumulation. Therefore, I added cleanup code that deletes the checkpoint directory on failure.

Thanks for the explanation, makes sense.

[smengcl]

Thanks for the patch. I have a few comments inline

[smengcl]

Could we also cover failures inside createCheckpoint()? It creates the checkpoint directory before opening, truncating, and reopening it, but this try/finally is entered only after createCheckpoint() returns. An exception during those steps still leaves a checkpoint directory under tmp_defrag, so repeated failures can reproduce the accumulation this patch is intended to fix. Please clean up once the DBCheckpoint exists and add failure coverage for this path.

[cchung100m]

Hi @smengcl

Thanks for detail suggestion. I had updated the part you mentioned.

[smengcl]

getCheckpoint() can return null after it catches an IOException. This line then throws NPE. The new cleanup block does not run, and triggerSnapshotDefragOnce() does not catch this exception. Pls report this failure as an IOException, delete any partial checkpoint directory, and add a test for this path.

[smengcl]

The tests do not cover the two new recovery paths. testCheckpointCleanupOnDefragFailure() stubs createCheckpoint(), so it does not run the new cleanup in that method. testCheckAndDefragAlreadyDefraggedSnapshot() returns version 0, so it does not run the deletion retry. Please add a test for a failure after checkpoint creation and one test that verifies deletion is retried for an already defragmented snapshot.

[smengcl]

Thanks @cchung100m for the patch.

[Copilot]

Pull request overview

This PR hardens Ozone Manager’s snapshot defragmentation flow to ensure temporary RocksDB checkpoint directories are cleaned up on failures, preventing long-running clusters from accumulating orphaned checkpoint directories and exhausting disk space.

Changes:

• Add failure-safe checkpoint creation and partial-checkpoint cleanup when DBStore#getCheckpoint(...) returns null.
• Ensure checkAndDefragSnapshot(...) cleans up the checkpoint directory in a finally block when defrag does not complete successfully, and guards close()/old-checkpoint deletion to avoid bypassing cleanup.
• Add a parameterized test validating checkpoint directory cleanup on both full and incremental defrag failures.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/defrag/SnapshotDefragService.java	Adds multi-layer exception handling and cleanup logic for failed/partial checkpoints during snapshot defrag.
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/defrag/TestSnapshotDefragService.java	Adds a new test verifying temporary checkpoint directories are deleted when defrag fails.

Suppressed comments (1)

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/defrag/SnapshotDefragService.java:807

• PR description says cleanup failures are tracked via incNumSnapshotDefragFails(), but this cleanup failure path only logs. If the intent is to surface orphaned-checkpoint cleanup failures via metrics, increment the counter here too (or adjust the PR description).

        } catch (IOException cleanupException) {
          LOG.error("Failed to delete checkpoint directory {} for snapshot: {} (ID: {}). " +
              "Disk space may not be freed. Manual cleanup may be required.",
              checkpointLocation, snapshotInfo.getTableKey(), snapshotInfo.getSnapshotId(), cleanupException);
        }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[smengcl]

Copilot comments look legit. Pls address them as well.