HDDS-15828. Fix flaky TestDirectoryDeletingServiceWithFSO snapshot cleanup - #11212
HDDS-15828. Fix flaky TestDirectoryDeletingServiceWithFSO snapshot cleanup#11212arunsarin85 wants to merge 4 commits into
Conversation
…eanup. Wait for SnapshotDeletingService to purge deleted snapshots before snapshot tests return on the shared mini-cluster, so later tests do not chain off stale global snapshot entries. Co-authored-by: Cursor <cursoragent@cursor.com> Change-Id: I57ec596cd5870b68f21ff9d9a89850ea9832f56e
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…waitForSnapshotsPurged
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for picking this up! @arunsarin85
I noticed ·waitForSnapshotsPurged()` resumes the service and then drives it manually, which may race with the background run. I tested a few local runs and saw the two overlap.
I also left a few inline suggestions for existing cleanup gaps.
| waitForSnapshotsPurged(snapshotInfoTable, snapshotCountAfterTest); | ||
| snapshotCountAfterTest = -1; | ||
| } | ||
| if (pendingResumeDds != null) { |
There was a problem hiding this comment.
If this times out or throws, pendingResumeDds.resume() is skipped. Would it make sense to wrap this in a try/finally?
There was a problem hiding this comment.
The snapshot-wait + snapshot-delete block is now in try/finally. pendingResumeDds.resume() and cleanupTables() are in the finally block, so they execute unconditionally even if waitForSnapshotsPurged times out or throws.
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to run SnapshotDeletingService purge task", e); | ||
| } | ||
| }, 1000, 120000); |
There was a problem hiding this comment.
Would reusing assertTableRowCount() here work? It already polls the same count every second for two minutes, keeping the test thread out of the service.
cluster.getOzoneManager().getKeyManager().getSnapshotDeletingService().resume();
assertTableRowCount(snapshotInfoTable, expectedCount);There was a problem hiding this comment.
runPeriodicalTaskNow() and background SDS waitForSnapshotsPurged is now just two lines
| client.getObjectStore().deleteSnapshot(volumeName, bucketName, "snap1"); | ||
| cleanupTables(); | ||
| snapshotCountAfterTest = 0; |
There was a problem hiding this comment.
Should these flags be set right after the snapshot is created instead? Set here, a failure earlier in the test may leave snap1 and the dirty tables behind, which is the case the teardown is meant to cover.
There was a problem hiding this comment.
snapshotToDeleteInCleanup = "snap1", snapshotCountAfterTest = 0, and needsTableCleanup = true are now set immediately after createSnapshot("snap1"). The teardown attempts deleteSnapshot (ignoring "already deleted" errors), then waits for the table to reach 0, then calls cleanupTables. A mid-test failure no longer leaves snap1 or dirty tables behind.
| @@ -571,10 +590,11 @@ public void testAOSKeyDeletingWithSnapshotCreateParallelExecution() | |||
| Table<String, SnapshotInfo> snapshotInfoTable = omMetadataManager.getSnapshotInfoTable(); | |||
| Table<String, OmKeyInfo> deletedDirTable = omMetadataManager.getDeletedDirTable(); | |||
| Table<String, String> renameTable = omMetadataManager.getSnapshotRenamedTable(); | |||
| cluster.getOzoneManager().getKeyManager().getSnapshotDeletingService().shutdown(); | |||
| cluster.getOzoneManager().getKeyManager().getSnapshotDeletingService().suspend(); | |||
There was a problem hiding this comment.
Could we track the need to resume SnapshotDeletingService immediately after suspending it, as we do with pendingResumeDds? snapshotCountAfterTest is assigned later, so an exception in between would cause @AfterEach to skip resuming SDS.
There was a problem hiding this comment.
pendingSdsResume = true is now set on the very next line after sds.suspend(), matching the pattern used for pendingResumeDds. The @AfterEach resumes SDS first (before attempting the snapshot count wait), so SDS is guaranteed to be resumed
3 participants
What changes were proposed in this pull request?
Addresses review comments on #10726 (jojochuang's original patch for HDDS-15828).
Previously,
testAOSKeyDeletingWithSnapshotCreateParallelExecutionsuspended bothSnapshotDeletingServiceandDirectoryDeletingServicebut only resumed them on thesuccess path. If an assertion failed before the resume calls, later tests in the shared
static cluster ran with services still suspended, causing cascading flakiness.
Please describe your PR in detail:
snapshotCountAfterTest,pendingResumeDds,needsTableCleanup) to track per-test cleanup state.@AfterEach cleanup()to: resumeSnapshotDeletingService(viawaitForSnapshotsPurged) andDirectoryDeletingServiceif suspended, andcall
cleanupTables()if needed — guaranteeing cleanup runs even on test failure.waitForSnapshotsPurged+dirDeletingService.resume()callsout of
testAOSKeyDeletingWithSnapshotCreateParallelExecutionand into@AfterEach.waitForSnapshotsPurged+cleanupTables()callsout of
testDirDeletedTableCleanUpForSnapshotand into@AfterEach.awaitDoubleBufferFlush()beforewaitForSnapshotsPurgedin
testDirDeletedTableCleanUpForSnapshot(suggested by SaketaChalamchala).waitForSnapshotsPurgedlambda (checkstyle).assertTableRowCountafterwaitForinwaitForSnapshotsPurged—
waitForalready guarantees the condition before returning, calling it againdoubles the potential wait time (up to 4 minutes on slow runs).
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15828
How was this patch tested?
https://github.com/arunsarin85/ozone/actions/runs/33992835360