You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "smengcl (via GitHub)" <gi...@apache.org> on 2026/08/13 05:18:24 UTC
[PR] DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
smengcl opened a new pull request, #11009:
URL: https://github.com/apache/ozone/pull/11009
## What changes were proposed in this pull request?
DirectoryDeletingService (DDS) processes snapshots with parallel directory workers. The task thread retains the current snapshot DB handle, while each worker can retain previous-snapshot handles through reclaimable filters. Those handles hold striped `SNAPSHOT_DB_LOCK` read locks.
Before this change, a worker could synchronously submit `PurgeDirectories` while retaining its previous-snapshot handles, or DDS could submit its final snapshot-property request while retaining the task-owned current handle. If `OMDoubleBufferFlushThread` was applying an earlier snapshot purge that needed the colliding write-lock stripe and the unflushed transaction limit was reached, both sides could wait indefinitely.
This patch preserves per-snapshot worker parallelism while enforcing this order:
1. Each worker closes its previous-snapshot DB handles after it finishes reading.
2. DDS waits for every worker to reach that point, closes its current snapshot handle and iterator, then releases workers to submit requests.
3. Workers retain their snapshot-GC locks until their requests complete.
Snapshot task setup is serialized before the task-owned current handle is opened, preventing a second snapshot task from waiting while retaining such a handle. The worker barrier also handles a rejected executor submission during reconfiguration: DDS accounts for unscheduled workers, releases workers that already started, and retries the work in a later run.
The patch adds a deterministic circular-wait regression with one snapshot DB lock stripe and one unflushed transaction. It also verifies that snapshot work retains the configured worker parallelism and that a rejected worker submission does not strand an accepted worker.
Generated-by: Codex (GPT-5)
## What is the link to the Apache JIRA
[HDDS-16164](https://issues.apache.org/jira/browse/HDDS-16164)
## How was this patch tested?
- `mvn -pl :ozone-manager test -Dtest=TestDirectoryDeletingService -DskipShade -DskipRecon -DskipDocs` (6 tests passed)
- `mvn -pl :ozone-manager -am install -DskipTests -DskipShade -DskipRecon -DskipDocs` (27-module reactor, 497 goals)
- `mvn -pl :ozone-integration-test test -Dtest=TestDirectoryDeletingServiceWithFSO#testAOSKeyDeletingWithSnapshotCreateParallelExecution -DskipShade -DskipRecon -DskipDocs` (1 test passed)
- `mvn -pl :ozone-manager checkstyle:check -DskipTests -DskipShade -DskipRecon -DskipDocs` (0 violations)
- `git diff --check`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #11009:
URL: https://github.com/apache/ozone/pull/11009#discussion_r4211952760
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -670,44 +685,107 @@ void processDeletedDirsForStore(SnapshotInfo currentSnapshotInfo, KeyManager key
UUID expectedPreviousSnapshotId = currentSnapshotInfo == null ?
snapshotChainManager.getLatestGlobalSnapshotId() :
SnapshotUtils.getPreviousSnapshotId(currentSnapshotInfo, snapshotChainManager);
- Map<UUID, Pair<Long, Long>> exclusiveSizeMap = Maps.newConcurrentMap();
+ processedAllDeletedDirs = runDeletionWorkers(currentSnapshotInfo, keyManager, dirSupplier, currentSnapshot,
+ expectedPreviousSnapshotId, exclusiveSizeMap, rnCnt, remainNum);
+ }
+
+ // If AOS or all directories have been processed for snapshot, update snapshot size delta and deep clean flag
+ // if it is a snapshot. All snapshot DB iterators and handles have been closed before this synchronous request.
+ if (processedAllDeletedDirs) {
+ List<OzoneManagerProtocolProtos.SetSnapshotPropertyRequest> setSnapshotPropertyRequests = new ArrayList<>();
+
+ for (Map.Entry<UUID, Pair<Long, Long>> entry : exclusiveSizeMap.entrySet()) {
+ UUID snapshotID = entry.getKey();
+ long exclusiveSize = entry.getValue().getLeft();
+ long exclusiveReplicatedSize = entry.getValue().getRight();
+ setSnapshotPropertyRequests.add(getSetSnapshotRequestUpdatingExclusiveSize(
+ exclusiveSize, exclusiveReplicatedSize, snapshotID));
+ }
+
+ // Updating directory deep clean flag of snapshot.
+ if (currentSnapshotInfo != null) {
+ setSnapshotPropertyRequests.add(OzoneManagerProtocolProtos.SetSnapshotPropertyRequest.newBuilder()
+ .setSnapshotKey(snapshotTableKey)
+ .setDeepCleanedDeletedDir(true)
+ .build());
+ }
+ submitSetSnapshotRequests(setSnapshotPropertyRequests);
+ }
+ }
- CompletableFuture<Boolean> processedAllDeletedDirs = CompletableFuture.completedFuture(true);
- final int parallelThreads = numberOfParallelThreadsPerStore.get();
- for (int i = 0; i < parallelThreads; i++) {
+ /**
+ * Waits for snapshot workers to close DB handles before opening the submission gate.
+ * The workers retain their GC locks through submission; the coordinator closes its own iterator and handle.
+ * AOS has no task-owned snapshot handle, so its workers can close their handles and submit independently.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private boolean runDeletionWorkers(SnapshotInfo currentSnapshotInfo, KeyManager keyManager,
+ DeletedDirSupplier dirSupplier, UncheckedAutoCloseableSupplier<OmSnapshot> currentSnapshot,
+ UUID expectedPreviousSnapshotId, Map<UUID, Pair<Long, Long>> exclusiveSizeMap, long rnCnt, int remainNum)
+ throws ExecutionException, InterruptedException {
+ int parallelThreads = numberOfParallelThreadsPerStore.get();
+ CountDownLatch snapshotDbHandlesClosed = currentSnapshotInfo == null ? null : new CountDownLatch(parallelThreads);
+ CountDownLatch submitRequests = new CountDownLatch(1);
+ CompletableFuture<Boolean> workersCompleted = CompletableFuture.completedFuture(true);
+ for (int i = 0; i < parallelThreads; i++) {
+ AtomicBoolean workerReady = new AtomicBoolean();
+ try {
CompletableFuture<Boolean> future = CompletableFuture.supplyAsync(() -> {
try {
return processDeletedDirectories(currentSnapshotInfo, keyManager, dirSupplier,
- expectedPreviousSnapshotId, exclusiveSizeMap, rnCnt, remainNum);
+ expectedPreviousSnapshotId, exclusiveSizeMap, rnCnt, remainNum, () -> {
+ if (snapshotDbHandlesClosed != null) {
+ signalWorkerReady(workerReady, snapshotDbHandlesClosed);
+ if (awaitUninterruptibly(submitRequests)) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ });
} catch (Throwable e) {
return false;
+ } finally {
+ if (snapshotDbHandlesClosed != null) {
+ signalWorkerReady(workerReady, snapshotDbHandlesClosed);
+ }
}
}, isThreadPoolActive(deletionThreadPool) ? deletionThreadPool : ForkJoinPool.commonPool());
Review Comment:
Thanks. Agreed. Removed the fallback so a stopped DDS executor reaches the existing rejection handling. The regression now covers partial rejection and an already-stopped executor with three configured workers. Both cases pass with common-pool parallelism set to two.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #11009:
URL: https://github.com/apache/ozone/pull/11009#discussion_r3859968365
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -824,12 +920,25 @@ public BackgroundTaskResult call() {
} else if (!isPreviousPurgeTransactionFlushed()) {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot = snapInfo == null ? null :
- omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(), snapInfo.getBucketName(),
- snapInfo.getName())) {
- KeyManager keyManager = snapInfo == null ? getOzoneManager().getKeyManager()
- : omSnapshot.get().getKeyManager();
- processDeletedDirsForStore(snapInfo, keyManager, run, pathLimitPerTask);
+ boolean snapshotProcessingLockAcquired = false;
Review Comment:
code is complex and not easy to understand. Suggest to refactor this method.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -639,7 +648,8 @@ private OzoneManagerProtocolProtos.SetSnapshotPropertyRequest getSetSnapshotRequ
* @param keyManager KeyManager of the underlying store.
*/
@VisibleForTesting
- void processDeletedDirsForStore(SnapshotInfo currentSnapshotInfo, KeyManager keyManager, long rnCnt, int remainNum)
+ void processDeletedDirsForStore(SnapshotInfo currentSnapshotInfo, KeyManager keyManager,
Review Comment:
code is complex and not easy to understand. Suggest to refactor this method.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
Review Comment:
Unrelated nit: this javadoc lacks description of the method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #11009:
URL: https://github.com/apache/ozone/pull/11009#discussion_r3772642279
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -824,12 +920,25 @@ public BackgroundTaskResult call() {
} else if (!isPreviousPurgeTransactionFlushed()) {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot = snapInfo == null ? null :
- omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(), snapInfo.getBucketName(),
- snapInfo.getName())) {
- KeyManager keyManager = snapInfo == null ? getOzoneManager().getKeyManager()
- : omSnapshot.get().getKeyManager();
- processDeletedDirsForStore(snapInfo, keyManager, run, pathLimitPerTask);
+ boolean snapshotProcessingLockAcquired = false;
+ try {
+ if (snapInfo != null) {
+ // Serialize snapshot setup before opening the snapshot DB. Workers still process each snapshot in
+ // parallel, but another snapshot task cannot wait here while retaining a task-owned DB handle.
+ snapshotProcessingLock.lockInterruptibly();
+ snapshotProcessingLockAcquired = true;
+ }
+ try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot = snapInfo == null ? null :
+ omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(), snapInfo.getBucketName(),
+ snapInfo.getName())) {
+ KeyManager keyManager = snapInfo == null ? getOzoneManager().getKeyManager()
+ : omSnapshot.get().getKeyManager();
+ processDeletedDirsForStore(snapInfo, keyManager, omSnapshot, run, pathLimitPerTask);
+ }
+ } finally {
+ if (snapshotProcessingLockAcquired) {
+ snapshotProcessingLock.unlock();
+ }
}
Review Comment:
`snapshotProcessingLock` is held for the entire snapshot deletion task (including directory scanning and synchronous OM requests) even though the comment says it only serializes snapshot setup. With the BackgroundService pool size equal to the configured deletion threads, multiple snapshot tasks can end up blocking on this lock and occupy the service thread pool, delaying AOS work and other snapshots unnecessarily.
Consider limiting the lock scope to only the `getActiveSnapshot(...)` open (or other setup that must be serialized) and releasing it immediately afterward, before the potentially long-running `processDeletedDirsForStore(...)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl merged PR #11009:
URL: https://github.com/apache/ozone/pull/11009
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "rich7420 (via GitHub)" <gi...@apache.org>.
rich7420 commented on code in PR #11009:
URL: https://github.com/apache/ozone/pull/11009#discussion_r4129051178
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -670,44 +685,107 @@ void processDeletedDirsForStore(SnapshotInfo currentSnapshotInfo, KeyManager key
UUID expectedPreviousSnapshotId = currentSnapshotInfo == null ?
snapshotChainManager.getLatestGlobalSnapshotId() :
SnapshotUtils.getPreviousSnapshotId(currentSnapshotInfo, snapshotChainManager);
- Map<UUID, Pair<Long, Long>> exclusiveSizeMap = Maps.newConcurrentMap();
+ processedAllDeletedDirs = runDeletionWorkers(currentSnapshotInfo, keyManager, dirSupplier, currentSnapshot,
+ expectedPreviousSnapshotId, exclusiveSizeMap, rnCnt, remainNum);
+ }
+
+ // If AOS or all directories have been processed for snapshot, update snapshot size delta and deep clean flag
+ // if it is a snapshot. All snapshot DB iterators and handles have been closed before this synchronous request.
+ if (processedAllDeletedDirs) {
+ List<OzoneManagerProtocolProtos.SetSnapshotPropertyRequest> setSnapshotPropertyRequests = new ArrayList<>();
+
+ for (Map.Entry<UUID, Pair<Long, Long>> entry : exclusiveSizeMap.entrySet()) {
+ UUID snapshotID = entry.getKey();
+ long exclusiveSize = entry.getValue().getLeft();
+ long exclusiveReplicatedSize = entry.getValue().getRight();
+ setSnapshotPropertyRequests.add(getSetSnapshotRequestUpdatingExclusiveSize(
+ exclusiveSize, exclusiveReplicatedSize, snapshotID));
+ }
+
+ // Updating directory deep clean flag of snapshot.
+ if (currentSnapshotInfo != null) {
+ setSnapshotPropertyRequests.add(OzoneManagerProtocolProtos.SetSnapshotPropertyRequest.newBuilder()
+ .setSnapshotKey(snapshotTableKey)
+ .setDeepCleanedDeletedDir(true)
+ .build());
+ }
+ submitSetSnapshotRequests(setSnapshotPropertyRequests);
+ }
+ }
- CompletableFuture<Boolean> processedAllDeletedDirs = CompletableFuture.completedFuture(true);
- final int parallelThreads = numberOfParallelThreadsPerStore.get();
- for (int i = 0; i < parallelThreads; i++) {
+ /**
+ * Waits for snapshot workers to close DB handles before opening the submission gate.
+ * The workers retain their GC locks through submission; the coordinator closes its own iterator and handle.
+ * AOS has no task-owned snapshot handle, so its workers can close their handles and submit independently.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private boolean runDeletionWorkers(SnapshotInfo currentSnapshotInfo, KeyManager keyManager,
+ DeletedDirSupplier dirSupplier, UncheckedAutoCloseableSupplier<OmSnapshot> currentSnapshot,
+ UUID expectedPreviousSnapshotId, Map<UUID, Pair<Long, Long>> exclusiveSizeMap, long rnCnt, int remainNum)
+ throws ExecutionException, InterruptedException {
+ int parallelThreads = numberOfParallelThreadsPerStore.get();
+ CountDownLatch snapshotDbHandlesClosed = currentSnapshotInfo == null ? null : new CountDownLatch(parallelThreads);
+ CountDownLatch submitRequests = new CountDownLatch(1);
+ CompletableFuture<Boolean> workersCompleted = CompletableFuture.completedFuture(true);
+ for (int i = 0; i < parallelThreads; i++) {
+ AtomicBoolean workerReady = new AtomicBoolean();
+ try {
CompletableFuture<Boolean> future = CompletableFuture.supplyAsync(() -> {
try {
return processDeletedDirectories(currentSnapshotInfo, keyManager, dirSupplier,
- expectedPreviousSnapshotId, exclusiveSizeMap, rnCnt, remainNum);
+ expectedPreviousSnapshotId, exclusiveSizeMap, rnCnt, remainNum, () -> {
+ if (snapshotDbHandlesClosed != null) {
+ signalWorkerReady(workerReady, snapshotDbHandlesClosed);
+ if (awaitUninterruptibly(submitRequests)) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ });
} catch (Throwable e) {
return false;
+ } finally {
+ if (snapshotDbHandlesClosed != null) {
+ signalWorkerReady(workerReady, snapshotDbHandlesClosed);
+ }
}
}, isThreadPoolActive(deletionThreadPool) ? deletionThreadPool : ForkJoinPool.commonPool());
Review Comment:
This fallback can deadlock the snapshot coordinator during shutdown. With three workers and common-pool parallelism two, two workers wait at the submission gate while the third remains queued. I reproduced this with a stopped DDS executor and a real snapshot. Please submit directly to the DDS executor and use the rejection handling below; the same test passes with that change.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestDirectoryDeletingService.java:
##########
@@ -383,4 +413,254 @@ void testPurgeDirectoriesGroupedByBucketPerTransaction() throws Exception {
bucketIdA, bucketIdB);
}
+ /**
+ * Regression test for HDDS-16164. Snapshot workers must wait for the task-owned handle to close before submitting.
+ * AOS workers own their handles and can finish scanning while a sibling waits for double-buffer capacity.
+ */
+ @ParameterizedTest(name = "activeStore={0}")
+ @ValueSource(booleans = {false, true})
+ @DisplayName("DirectoryDeletingService must not deadlock at the unflushed transaction limit")
+ void testNoUnflushedTransactionDeadlock(boolean activeStore) throws Exception {
+ int threadCount = 3;
+ OzoneConfiguration conf = createConfAndInitValues(threadCount);
+ conf.setInt(OMConfigKeys.OZONE_OM_UNFLUSHED_TRANSACTION_MAX_COUNT, 1);
+ conf.setInt(OZONE_PATH_DELETING_LIMIT_PER_TASK, 1);
+ conf.setInt(OZONE_MANAGER_STRIPED_LOCK_SIZE_PREFIX + "snapshot_db_lock", 1);
+ conf.setTimeDuration(OZONE_BLOCK_DELETING_SERVICE_INTERVAL, 1, TimeUnit.HOURS);
+ conf.setTimeDuration(OZONE_SNAPSHOT_DELETING_SERVICE_INTERVAL, 1, TimeUnit.HOURS);
+ conf.setTimeDuration(OZONE_DIR_DELETING_SERVICE_INTERVAL, 1, TimeUnit.HOURS);
+
+ OmTestManagers managers = new OmTestManagers(conf);
+ om = managers.getOzoneManager();
+ KeyManager keyManager = managers.getKeyManager();
+ OMMetadataManager metadataManager = managers.getMetadataManager();
+ OzoneManagerProtocol writeClient = managers.getWriteClient();
+ keyManager.getDirDeletingService().shutdown();
+
+ OMRequestTestUtils.addVolumeToOM(metadataManager,
+ OmVolumeArgs.newBuilder().setOwnerName("o").setAdminName("a").setVolume(volumeName).build());
+ OMRequestTestUtils.addBucketToOM(metadataManager,
+ OmBucketInfo.newBuilder().setVolumeName(volumeName).setBucketName(bucketName)
+ .setObjectID(1).setBucketLayout(BucketLayout.FILE_SYSTEM_OPTIMIZED).build());
+
+ String purgeSnapshotName = "purge";
+ writeClient.createSnapshot(volumeName, bucketName, purgeSnapshotName);
+ String previousSnapshotName = "previous";
+ writeClient.createSnapshot(volumeName, bucketName, previousSnapshotName);
+ String deepCleanSnapshotName = "deep-clean";
+ if (activeStore) {
+ writeClient.createSnapshot(volumeName, bucketName, deepCleanSnapshotName);
+ }
+ om.awaitDoubleBufferFlush();
+
+ addDeletedDirectories(metadataManager, threadCount);
+ if (!activeStore) {
+ writeClient.createSnapshot(volumeName, bucketName, deepCleanSnapshotName);
+ }
+ om.awaitDoubleBufferFlush();
+
+ String deepCleanKey = SnapshotInfo.getTableKey(volumeName, bucketName, deepCleanSnapshotName);
+ SnapshotInfo deepCleanInfo = metadataManager.getSnapshotInfoTable().get(deepCleanKey);
+ assertNotNull(deepCleanInfo);
+ awaitSnapshotFlushed(metadataManager, deepCleanInfo);
+
+ writeClient.deleteSnapshot(volumeName, bucketName, purgeSnapshotName);
+ om.awaitDoubleBufferFlush();
+ String purgeKey = SnapshotInfo.getTableKey(volumeName, bucketName, purgeSnapshotName);
+ assertNotNull(metadataManager.getSnapshotInfoTable().get(purgeKey));
+
+ CountDownLatch ddsAtSubmit = new CountDownLatch(1);
+ CountDownLatch ddsMayProceed = new CountDownLatch(1);
+ DirectoryDeletingService service = Mockito.spy(new DirectoryDeletingService(1, TimeUnit.HOURS, 1,
+ om, conf, threadCount, true) {
+ @Override
+ protected OzoneManagerProtocolProtos.OMResponse submitRequest(
+ OzoneManagerProtocolProtos.OMRequest omRequest) throws ServiceException {
+ assertThat(omRequest.getCmdType()).isIn(
+ OzoneManagerProtocolProtos.Type.PurgeDirectories,
+ OzoneManagerProtocolProtos.Type.SetSnapshotProperty);
+ ddsAtSubmit.countDown();
+ try {
+ ddsMayProceed.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new ServiceException(e);
+ }
+ return super.submitRequest(omRequest);
+ }
+ });
+
+ CountDownLatch workersPrepared = new CountDownLatch(threadCount);
+ CountDownLatch finishScanning = new CountDownLatch(1);
+ AtomicBoolean pauseScan = new AtomicBoolean();
+ Mockito.doAnswer(invocation -> {
+ workersPrepared.countDown();
+ if (pauseScan.compareAndSet(false, true)) {
+ // Keep this worker's previous-snapshot handle open while the other workers finish scanning.
+ assertThat(metadataManager.getLock().getReadHoldCount(SNAPSHOT_DB_LOCK,
+ deepCleanInfo.getSnapshotId().toString())).isPositive();
+ assertTrue(finishScanning.await(30, TimeUnit.SECONDS));
+ }
+ return invocation.callRealMethod();
+ }).when(service).optimizeDirDeletesAndSubmitRequest(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(),
+ Mockito.anyList(), Mockito.anyList(), Mockito.any(), Mockito.anyLong(), Mockito.any(), Mockito.any(),
+ Mockito.any(), Mockito.anyMap(), Mockito.any(), Mockito.anyLong(), Mockito.any(), Mockito.any());
+
+ OzoneManagerDoubleBuffer doubleBuffer =
+ om.getOmRatisServer().getOmStateMachine().getOzoneManagerDoubleBuffer();
+ Semaphore unflushedTransactions =
+ (Semaphore) HddsWhiteboxTestUtils.getInternalState(doubleBuffer, "unFlushedTransactions");
+ ThreadPoolExecutor deletionThreadPool = service.getDeletionThreadPool();
+ long completedTaskCountBefore = deletionThreadPool.getCompletedTaskCount();
+ ExecutorService executor = Executors.newFixedThreadPool(2);
+ try {
+ Future<?> ddsFuture = service.getExecutorService().submit(
+ () -> service.new DirDeletingTask(activeStore ? null : deepCleanInfo.getSnapshotId()).call());
+ assertTrue(workersPrepared.await(30, TimeUnit.SECONDS), "Expected all configured workers to scan in parallel");
+ if (!activeStore) {
+ assertThat(ddsAtSubmit.await(200, TimeUnit.MILLISECONDS))
+ .as("snapshot workers must wait for the task-owned snapshot handle to close").isFalse();
+ finishScanning.countDown();
+ }
+ assertTrue(ddsAtSubmit.await(30, TimeUnit.SECONDS), "DirDeletingTask never reached submitRequest");
+
+ CountDownLatch nextStoreStarted = new CountDownLatch(1);
+ Future<?> nextStore = service.getExecutorService().submit(nextStoreStarted::countDown);
+ assertThat(nextStoreStarted.await(200, TimeUnit.MILLISECONDS))
+ .as("the next store must not open DB handles while workers are submitting requests").isFalse();
+
+ AtomicBoolean purgeDone = new AtomicBoolean();
+ OzoneManagerProtocolProtos.OMRequest purgeRequest = OzoneManagerProtocolProtos.OMRequest.newBuilder()
+ .setCmdType(OzoneManagerProtocolProtos.Type.SnapshotPurge)
+ .setSnapshotPurgeRequest(OzoneManagerProtocolProtos.SnapshotPurgeRequest.newBuilder()
+ .addSnapshotDBKeys(purgeKey))
+ .setClientId(ClientId.randomId().toString())
+ .build();
+ Future<?> purgeFuture = executor.submit(() -> {
+ OzoneManagerRatisUtils.submitRequest(om, purgeRequest, ClientId.randomId(), 1L);
+ purgeDone.set(true);
+ return null;
+ });
+ GenericTestUtils.waitFor(
+ () -> unflushedTransactions.availablePermits() == 0 || purgeDone.get(), 50, 30000);
+
+ ddsMayProceed.countDown();
+ if (activeStore) {
+ GenericTestUtils.waitFor(unflushedTransactions::hasQueuedThreads, 50, 10000);
+ }
+ // An AOS worker can finish scanning and close its handle even while a sibling waits for flush capacity.
+ finishScanning.countDown();
+ try {
+ ddsFuture.get(60, TimeUnit.SECONDS);
+ } catch (TimeoutException e) {
+ fail("DirectoryDeletingService retained a snapshot DB read lock across its synchronous OM request");
+ }
+ purgeFuture.get(60, TimeUnit.SECONDS);
+ nextStore.get(10, TimeUnit.SECONDS);
+ om.awaitDoubleBufferFlush();
+ GenericTestUtils.waitFor(() -> deletionThreadPool.getCompletedTaskCount() ==
+ completedTaskCountBefore + threadCount, 100, 5000);
+ assertThat(deletionThreadPool.getCompletedTaskCount() - completedTaskCountBefore)
+ .as("store processing should use all configured deletion workers").isEqualTo(threadCount);
+ assertThat(service.getDeletedDirsCount()).isEqualTo(threadCount);
+ } finally {
+ finishScanning.countDown();
+ ddsMayProceed.countDown();
+ executor.shutdownNow();
+ service.shutdown();
+ }
+ }
+
+ @Test
+ @DisplayName("DirectoryDeletingService releases snapshot workers when submission is rejected")
+ void testRejectedSnapshotWorkerSubmission() throws Exception {
+ OzoneConfiguration conf = createConfAndInitValues(2);
+ OmTestManagers managers = new OmTestManagers(conf);
+ om = managers.getOzoneManager();
+ KeyManager keyManager = managers.getKeyManager();
+ OMMetadataManager metadataManager = managers.getMetadataManager();
+ OzoneManagerProtocol writeClient = managers.getWriteClient();
+ keyManager.getDirDeletingService().shutdown();
+
+ OMRequestTestUtils.addVolumeToOM(metadataManager,
+ OmVolumeArgs.newBuilder().setOwnerName("o").setAdminName("a").setVolume(volumeName).build());
+ OMRequestTestUtils.addBucketToOM(metadataManager,
+ OmBucketInfo.newBuilder().setVolumeName(volumeName).setBucketName(bucketName)
+ .setObjectID(1).setBucketLayout(BucketLayout.FILE_SYSTEM_OPTIMIZED).build());
+ String snapshotName = "snapshot";
+ writeClient.createSnapshot(volumeName, bucketName, snapshotName);
+ om.awaitDoubleBufferFlush();
+
+ String snapshotKey = SnapshotInfo.getTableKey(volumeName, bucketName, snapshotName);
+ SnapshotInfo snapshotInfo = metadataManager.getSnapshotInfoTable().get(snapshotKey);
+ assertNotNull(snapshotInfo);
+ awaitSnapshotFlushed(metadataManager, snapshotInfo);
+
+ DirectoryDeletingService service = new DirectoryDeletingService(1, TimeUnit.HOURS, 1,
+ om, conf, 2, true);
+ RejectAfterFirstExecutor rejectionExecutor = new RejectAfterFirstExecutor();
+ HddsWhiteboxTestUtils.setInternalState(service, "deletionThreadPool", rejectionExecutor);
+ ExecutorService taskExecutor = Executors.newSingleThreadExecutor();
+ try {
+ Future<?> processFuture = taskExecutor.submit(() -> {
+ try (UncheckedAutoCloseableSupplier<OmSnapshot> snapshot = om.getOmSnapshotManager().getActiveSnapshot(
+ volumeName, bucketName, snapshotName)) {
+ service.new DirDeletingTask(snapshotInfo.getSnapshotId()).processDeletedDirsForStore(snapshotInfo,
+ snapshot.get().getKeyManager(), snapshot, 1, 1);
+ }
+ return null;
+ });
+ processFuture.get(10, TimeUnit.SECONDS);
+ assertThat(rejectionExecutor.getCompletedTaskCount()).isEqualTo(1);
Review Comment:
Please wait for the completed-task count with `GenericTestUtils.waitFor`, as the other tests here do. The future can complete before the executor increments the counter. Holding the worker in `afterExecute` reproduces `expected: 1, but was: 0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #11009:
URL: https://github.com/apache/ozone/pull/11009#issuecomment-5914680079
I pushed a proposed follow-up fix to my fork: [6138bd8](https://github.com/jojochuang/ozone/commit/6138bd81d235b2124cb5cd0015f48cf37be39aa6) on [`HDDS-16164-dds-worker-submission-fix`](https://github.com/jojochuang/ozone/tree/HDDS-16164-dds-worker-submission-fix). The commit is based on the current PR head, `4428cf8`, and can be cherry-picked.
The fix addresses the two outstanding worker-submission concerns:
- Submit directly to the DDS executor rather than falling back to the common pool. A stopped executor now reaches the existing rejection handling, avoiding the barrier deadlock when the configured worker count exceeds common-pool parallelism.
- Wait for the executor's completed-task count in the rejection test instead of assuming completion accounting has finished when the coordinator future returns.
The rejection test now covers both partial rejection and an already-stopped executor with three configured workers.
Validation on JDK 21: all 9 `TestDirectoryDeletingService` tests passed; both rejection cases also passed with `JAVA_TOOL_OPTIONS=-Djava.util.concurrent.ForkJoinPool.common.parallelism=2`; the repository checkstyle script and `git diff --check` passed. I did not rerun integration tests.
Generated-by: Codex
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #11009:
URL: https://github.com/apache/ozone/pull/11009#issuecomment-6348673113
Thanks @jojochuang @rich7420 @SaketaChalamchala for the reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #11009:
URL: https://github.com/apache/ozone/pull/11009#discussion_r3859945675
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -658,43 +670,94 @@ void processDeletedDirsForStore(SnapshotInfo currentSnapshotInfo, KeyManager key
UUID expectedPreviousSnapshotId = currentSnapshotInfo == null ?
snapshotChainManager.getLatestGlobalSnapshotId() :
SnapshotUtils.getPreviousSnapshotId(currentSnapshotInfo, snapshotChainManager);
- Map<UUID, Pair<Long, Long>> exclusiveSizeMap = Maps.newConcurrentMap();
-
- CompletableFuture<Boolean> processedAllDeletedDirs = CompletableFuture.completedFuture(true);
final int parallelThreads = numberOfParallelThreadsPerStore.get();
+ CountDownLatch snapshotDbHandlesClosed = currentSnapshotInfo == null ? null :
Review Comment:
**Possible remaining deadlock (AOS, cross-worker)**
The snapshot barrier is skipped when `currentSnapshotInfo == null`, but AOS still fans out `parallelThreads` workers (default 10). Each worker calls `closeSnapshotDbHandles()` in `beforeSubmit` before **its own** submit, yet there is no cross-worker gate like `snapshotDbHandlesClosed`.
So worker A can be blocked in synchronous `submitRequest(PurgeDirectories)` waiting for double-buffer capacity while worker B is still scanning with previous-snapshot `SNAPSHOT_DB` read locks open via `ReclaimableFilter.apply()`. That is the same circular wait class as HDDS-16164:
```
Worker A: closed own handles → submitRequest → waits for unflushed slot
Worker B: still scanning → holds SNAPSHOT_DB read lock
Purge flush: holds slot → needs SNAPSHOT_DB write on colliding stripe → waits on B
```
`KeyDeletingService` avoids this by closing all handles once before any submit on a single worker thread. Consider extending the barrier to AOS, or serializing AOS submit after all workers finish scanning (even if scan stays parallel). With `OZONE_THREAD_NUMBER_DIR_DELETION=1` this particular cross-worker variant goes away.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -824,12 +920,25 @@ public BackgroundTaskResult call() {
} else if (!isPreviousPurgeTransactionFlushed()) {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot = snapInfo == null ? null :
- omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(), snapInfo.getBucketName(),
- snapInfo.getName())) {
- KeyManager keyManager = snapInfo == null ? getOzoneManager().getKeyManager()
- : omSnapshot.get().getKeyManager();
- processDeletedDirsForStore(snapInfo, keyManager, run, pathLimitPerTask);
+ boolean snapshotProcessingLockAcquired = false;
Review Comment:
**Possible remaining deadlock (AOS task concurrent with snapshot purge)**
`snapshotProcessingLock` serializes snapshot-vs-snapshot tasks, but AOS (`snapshotId == null`) never takes it. `BackgroundService` still runs the AOS `DirDeletingTask` concurrently with snapshot deep-clean / purge work.
Even with the snapshot path fully fixed, an AOS worker can retain previous-snapshot `SNAPSHOT_DB` read locks while another thread's snapshot purge occupies the double buffer and its flush needs a colliding write lock:
```
AOS worker B: scanning → SNAPSHOT_DB read on snapshot S
Snapshot purge for S: holds unflushed slot → flush waits for write lock on S
AOS worker A: finished scan, closed handles → submitRequest blocks on slot
```
Worth tracking as a follow-up if we want DDS-wide protection, not just snapshot deep-clean. Options: coarse coordination between AOS and snapshot/purge paths, or reuse the same "all workers released DB handles before any submit" invariant for AOS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #11009:
URL: https://github.com/apache/ozone/pull/11009#discussion_r4211954628
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestDirectoryDeletingService.java:
##########
@@ -383,4 +413,254 @@ void testPurgeDirectoriesGroupedByBucketPerTransaction() throws Exception {
bucketIdA, bucketIdB);
}
+ /**
+ * Regression test for HDDS-16164. Snapshot workers must wait for the task-owned handle to close before submitting.
+ * AOS workers own their handles and can finish scanning while a sibling waits for double-buffer capacity.
+ */
+ @ParameterizedTest(name = "activeStore={0}")
+ @ValueSource(booleans = {false, true})
+ @DisplayName("DirectoryDeletingService must not deadlock at the unflushed transaction limit")
+ void testNoUnflushedTransactionDeadlock(boolean activeStore) throws Exception {
+ int threadCount = 3;
+ OzoneConfiguration conf = createConfAndInitValues(threadCount);
+ conf.setInt(OMConfigKeys.OZONE_OM_UNFLUSHED_TRANSACTION_MAX_COUNT, 1);
+ conf.setInt(OZONE_PATH_DELETING_LIMIT_PER_TASK, 1);
+ conf.setInt(OZONE_MANAGER_STRIPED_LOCK_SIZE_PREFIX + "snapshot_db_lock", 1);
+ conf.setTimeDuration(OZONE_BLOCK_DELETING_SERVICE_INTERVAL, 1, TimeUnit.HOURS);
+ conf.setTimeDuration(OZONE_SNAPSHOT_DELETING_SERVICE_INTERVAL, 1, TimeUnit.HOURS);
+ conf.setTimeDuration(OZONE_DIR_DELETING_SERVICE_INTERVAL, 1, TimeUnit.HOURS);
+
+ OmTestManagers managers = new OmTestManagers(conf);
+ om = managers.getOzoneManager();
+ KeyManager keyManager = managers.getKeyManager();
+ OMMetadataManager metadataManager = managers.getMetadataManager();
+ OzoneManagerProtocol writeClient = managers.getWriteClient();
+ keyManager.getDirDeletingService().shutdown();
+
+ OMRequestTestUtils.addVolumeToOM(metadataManager,
+ OmVolumeArgs.newBuilder().setOwnerName("o").setAdminName("a").setVolume(volumeName).build());
+ OMRequestTestUtils.addBucketToOM(metadataManager,
+ OmBucketInfo.newBuilder().setVolumeName(volumeName).setBucketName(bucketName)
+ .setObjectID(1).setBucketLayout(BucketLayout.FILE_SYSTEM_OPTIMIZED).build());
+
+ String purgeSnapshotName = "purge";
+ writeClient.createSnapshot(volumeName, bucketName, purgeSnapshotName);
+ String previousSnapshotName = "previous";
+ writeClient.createSnapshot(volumeName, bucketName, previousSnapshotName);
+ String deepCleanSnapshotName = "deep-clean";
+ if (activeStore) {
+ writeClient.createSnapshot(volumeName, bucketName, deepCleanSnapshotName);
+ }
+ om.awaitDoubleBufferFlush();
+
+ addDeletedDirectories(metadataManager, threadCount);
+ if (!activeStore) {
+ writeClient.createSnapshot(volumeName, bucketName, deepCleanSnapshotName);
+ }
+ om.awaitDoubleBufferFlush();
+
+ String deepCleanKey = SnapshotInfo.getTableKey(volumeName, bucketName, deepCleanSnapshotName);
+ SnapshotInfo deepCleanInfo = metadataManager.getSnapshotInfoTable().get(deepCleanKey);
+ assertNotNull(deepCleanInfo);
+ awaitSnapshotFlushed(metadataManager, deepCleanInfo);
+
+ writeClient.deleteSnapshot(volumeName, bucketName, purgeSnapshotName);
+ om.awaitDoubleBufferFlush();
+ String purgeKey = SnapshotInfo.getTableKey(volumeName, bucketName, purgeSnapshotName);
+ assertNotNull(metadataManager.getSnapshotInfoTable().get(purgeKey));
+
+ CountDownLatch ddsAtSubmit = new CountDownLatch(1);
+ CountDownLatch ddsMayProceed = new CountDownLatch(1);
+ DirectoryDeletingService service = Mockito.spy(new DirectoryDeletingService(1, TimeUnit.HOURS, 1,
+ om, conf, threadCount, true) {
+ @Override
+ protected OzoneManagerProtocolProtos.OMResponse submitRequest(
+ OzoneManagerProtocolProtos.OMRequest omRequest) throws ServiceException {
+ assertThat(omRequest.getCmdType()).isIn(
+ OzoneManagerProtocolProtos.Type.PurgeDirectories,
+ OzoneManagerProtocolProtos.Type.SetSnapshotProperty);
+ ddsAtSubmit.countDown();
+ try {
+ ddsMayProceed.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new ServiceException(e);
+ }
+ return super.submitRequest(omRequest);
+ }
+ });
+
+ CountDownLatch workersPrepared = new CountDownLatch(threadCount);
+ CountDownLatch finishScanning = new CountDownLatch(1);
+ AtomicBoolean pauseScan = new AtomicBoolean();
+ Mockito.doAnswer(invocation -> {
+ workersPrepared.countDown();
+ if (pauseScan.compareAndSet(false, true)) {
+ // Keep this worker's previous-snapshot handle open while the other workers finish scanning.
+ assertThat(metadataManager.getLock().getReadHoldCount(SNAPSHOT_DB_LOCK,
+ deepCleanInfo.getSnapshotId().toString())).isPositive();
+ assertTrue(finishScanning.await(30, TimeUnit.SECONDS));
+ }
+ return invocation.callRealMethod();
+ }).when(service).optimizeDirDeletesAndSubmitRequest(Mockito.anyLong(), Mockito.anyLong(), Mockito.anyLong(),
+ Mockito.anyList(), Mockito.anyList(), Mockito.any(), Mockito.anyLong(), Mockito.any(), Mockito.any(),
+ Mockito.any(), Mockito.anyMap(), Mockito.any(), Mockito.anyLong(), Mockito.any(), Mockito.any());
+
+ OzoneManagerDoubleBuffer doubleBuffer =
+ om.getOmRatisServer().getOmStateMachine().getOzoneManagerDoubleBuffer();
+ Semaphore unflushedTransactions =
+ (Semaphore) HddsWhiteboxTestUtils.getInternalState(doubleBuffer, "unFlushedTransactions");
+ ThreadPoolExecutor deletionThreadPool = service.getDeletionThreadPool();
+ long completedTaskCountBefore = deletionThreadPool.getCompletedTaskCount();
+ ExecutorService executor = Executors.newFixedThreadPool(2);
+ try {
+ Future<?> ddsFuture = service.getExecutorService().submit(
+ () -> service.new DirDeletingTask(activeStore ? null : deepCleanInfo.getSnapshotId()).call());
+ assertTrue(workersPrepared.await(30, TimeUnit.SECONDS), "Expected all configured workers to scan in parallel");
+ if (!activeStore) {
+ assertThat(ddsAtSubmit.await(200, TimeUnit.MILLISECONDS))
+ .as("snapshot workers must wait for the task-owned snapshot handle to close").isFalse();
+ finishScanning.countDown();
+ }
+ assertTrue(ddsAtSubmit.await(30, TimeUnit.SECONDS), "DirDeletingTask never reached submitRequest");
+
+ CountDownLatch nextStoreStarted = new CountDownLatch(1);
+ Future<?> nextStore = service.getExecutorService().submit(nextStoreStarted::countDown);
+ assertThat(nextStoreStarted.await(200, TimeUnit.MILLISECONDS))
+ .as("the next store must not open DB handles while workers are submitting requests").isFalse();
+
+ AtomicBoolean purgeDone = new AtomicBoolean();
+ OzoneManagerProtocolProtos.OMRequest purgeRequest = OzoneManagerProtocolProtos.OMRequest.newBuilder()
+ .setCmdType(OzoneManagerProtocolProtos.Type.SnapshotPurge)
+ .setSnapshotPurgeRequest(OzoneManagerProtocolProtos.SnapshotPurgeRequest.newBuilder()
+ .addSnapshotDBKeys(purgeKey))
+ .setClientId(ClientId.randomId().toString())
+ .build();
+ Future<?> purgeFuture = executor.submit(() -> {
+ OzoneManagerRatisUtils.submitRequest(om, purgeRequest, ClientId.randomId(), 1L);
+ purgeDone.set(true);
+ return null;
+ });
+ GenericTestUtils.waitFor(
+ () -> unflushedTransactions.availablePermits() == 0 || purgeDone.get(), 50, 30000);
+
+ ddsMayProceed.countDown();
+ if (activeStore) {
+ GenericTestUtils.waitFor(unflushedTransactions::hasQueuedThreads, 50, 10000);
+ }
+ // An AOS worker can finish scanning and close its handle even while a sibling waits for flush capacity.
+ finishScanning.countDown();
+ try {
+ ddsFuture.get(60, TimeUnit.SECONDS);
+ } catch (TimeoutException e) {
+ fail("DirectoryDeletingService retained a snapshot DB read lock across its synchronous OM request");
+ }
+ purgeFuture.get(60, TimeUnit.SECONDS);
+ nextStore.get(10, TimeUnit.SECONDS);
+ om.awaitDoubleBufferFlush();
+ GenericTestUtils.waitFor(() -> deletionThreadPool.getCompletedTaskCount() ==
+ completedTaskCountBefore + threadCount, 100, 5000);
+ assertThat(deletionThreadPool.getCompletedTaskCount() - completedTaskCountBefore)
+ .as("store processing should use all configured deletion workers").isEqualTo(threadCount);
+ assertThat(service.getDeletedDirsCount()).isEqualTo(threadCount);
+ } finally {
+ finishScanning.countDown();
+ ddsMayProceed.countDown();
+ executor.shutdownNow();
+ service.shutdown();
+ }
+ }
+
+ @Test
+ @DisplayName("DirectoryDeletingService releases snapshot workers when submission is rejected")
+ void testRejectedSnapshotWorkerSubmission() throws Exception {
+ OzoneConfiguration conf = createConfAndInitValues(2);
+ OmTestManagers managers = new OmTestManagers(conf);
+ om = managers.getOzoneManager();
+ KeyManager keyManager = managers.getKeyManager();
+ OMMetadataManager metadataManager = managers.getMetadataManager();
+ OzoneManagerProtocol writeClient = managers.getWriteClient();
+ keyManager.getDirDeletingService().shutdown();
+
+ OMRequestTestUtils.addVolumeToOM(metadataManager,
+ OmVolumeArgs.newBuilder().setOwnerName("o").setAdminName("a").setVolume(volumeName).build());
+ OMRequestTestUtils.addBucketToOM(metadataManager,
+ OmBucketInfo.newBuilder().setVolumeName(volumeName).setBucketName(bucketName)
+ .setObjectID(1).setBucketLayout(BucketLayout.FILE_SYSTEM_OPTIMIZED).build());
+ String snapshotName = "snapshot";
+ writeClient.createSnapshot(volumeName, bucketName, snapshotName);
+ om.awaitDoubleBufferFlush();
+
+ String snapshotKey = SnapshotInfo.getTableKey(volumeName, bucketName, snapshotName);
+ SnapshotInfo snapshotInfo = metadataManager.getSnapshotInfoTable().get(snapshotKey);
+ assertNotNull(snapshotInfo);
+ awaitSnapshotFlushed(metadataManager, snapshotInfo);
+
+ DirectoryDeletingService service = new DirectoryDeletingService(1, TimeUnit.HOURS, 1,
+ om, conf, 2, true);
+ RejectAfterFirstExecutor rejectionExecutor = new RejectAfterFirstExecutor();
+ HddsWhiteboxTestUtils.setInternalState(service, "deletionThreadPool", rejectionExecutor);
+ ExecutorService taskExecutor = Executors.newSingleThreadExecutor();
+ try {
+ Future<?> processFuture = taskExecutor.submit(() -> {
+ try (UncheckedAutoCloseableSupplier<OmSnapshot> snapshot = om.getOmSnapshotManager().getActiveSnapshot(
+ volumeName, bucketName, snapshotName)) {
+ service.new DirDeletingTask(snapshotInfo.getSnapshotId()).processDeletedDirsForStore(snapshotInfo,
+ snapshot.get().getKeyManager(), snapshot, 1, 1);
+ }
+ return null;
+ });
+ processFuture.get(10, TimeUnit.SECONDS);
+ assertThat(rejectionExecutor.getCompletedTaskCount()).isEqualTo(1);
Review Comment:
Added GenericTestUtils.waitFor before checking the completed-task count, matching the other tests here. The test expects one completed task after partial rejection and zero for a stopped executor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "SaketaChalamchala (via GitHub)" <gi...@apache.org>.
SaketaChalamchala commented on PR #11009:
URL: https://github.com/apache/ozone/pull/11009#issuecomment-6323561275
Thanks for the patch @smengcl. This change lgtm. Just a small follow up, could you add a tests for:
1. similar to KDS `testSnapshotDbHandleClosedBeforeSubmit` to assert that `SNAPSHOT_DB` locks are released before submitting purge requests?
2. A test where a deletion task worker throws (maybe in reclaimable filter) and assert that `processDeletedDirsForStore` does not hang and `SNAPSHOT_DB` locks are released
Also, PTAL at the CI failures and the merge conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #11009:
URL: https://github.com/apache/ozone/pull/11009#issuecomment-6324383352
Thanks @SaketaChalamchala . It is done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16164. DirectoryDeletingService could deadlock snapshot purge while retaining snapshot DB handles [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #11009:
URL: https://github.com/apache/ozone/pull/11009#discussion_r4069448758
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -824,12 +920,25 @@ public BackgroundTaskResult call() {
} else if (!isPreviousPurgeTransactionFlushed()) {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot = snapInfo == null ? null :
- omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(), snapInfo.getBucketName(),
- snapInfo.getName())) {
- KeyManager keyManager = snapInfo == null ? getOzoneManager().getKeyManager()
- : omSnapshot.get().getKeyManager();
- processDeletedDirsForStore(snapInfo, keyManager, run, pathLimitPerTask);
+ boolean snapshotProcessingLockAcquired = false;
+ try {
+ if (snapInfo != null) {
+ // Serialize snapshot setup before opening the snapshot DB. Workers still process each snapshot in
+ // parallel, but another snapshot task cannot wait here while retaining a task-owned DB handle.
+ snapshotProcessingLock.lockInterruptibly();
+ snapshotProcessingLockAcquired = true;
+ }
+ try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot = snapInfo == null ? null :
+ omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(), snapInfo.getBucketName(),
+ snapInfo.getName())) {
+ KeyManager keyManager = snapInfo == null ? getOzoneManager().getKeyManager()
+ : omSnapshot.get().getKeyManager();
+ processDeletedDirsForStore(snapInfo, keyManager, omSnapshot, run, pathLimitPerTask);
+ }
+ } finally {
+ if (snapshotProcessingLockAcquired) {
+ snapshotProcessingLock.unlock();
+ }
}
Review Comment:
The lock covers the whole task, and the setup wording was misleading. Releasing it immediately after `getActiveSnapshot()` would allow another task to retain a DB handle while waiting for the shared worker pool. The revision replaces the lock with one BackgroundService coordinator for AOS and snapshot tasks. The separate deletion pool retains the configured worker parallelism.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
Review Comment:
Added the method summary and documented the coordinator requirement and current-snapshot handle ownership.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -639,7 +648,8 @@ private OzoneManagerProtocolProtos.SetSnapshotPropertyRequest getSetSnapshotRequ
* @param keyManager KeyManager of the underlying store.
*/
@VisibleForTesting
- void processDeletedDirsForStore(SnapshotInfo currentSnapshotInfo, KeyManager keyManager, long rnCnt, int remainNum)
+ void processDeletedDirsForStore(SnapshotInfo currentSnapshotInfo, KeyManager keyManager,
Review Comment:
Extracted worker scheduling, handle closure, and completion waiting into `runDeletionWorkers()`. `processDeletedDirsForStore()` now handles store setup and final snapshot-property updates. Rejection and interruption handling remain together in the coordination method.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -824,12 +920,25 @@ public BackgroundTaskResult call() {
} else if (!isPreviousPurgeTransactionFlushed()) {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot = snapInfo == null ? null :
- omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(), snapInfo.getBucketName(),
- snapInfo.getName())) {
- KeyManager keyManager = snapInfo == null ? getOzoneManager().getKeyManager()
- : omSnapshot.get().getKeyManager();
- processDeletedDirsForStore(snapInfo, keyManager, run, pathLimitPerTask);
+ boolean snapshotProcessingLockAcquired = false;
Review Comment:
Removed the lock-acquisition flag and nested cleanup blocks. With one store coordinator, `call()` checks eligibility, opens the task-owned handle, and invokes store processing. The deletion-worker pool retains configured parallelism.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -658,43 +670,94 @@ void processDeletedDirsForStore(SnapshotInfo currentSnapshotInfo, KeyManager key
UUID expectedPreviousSnapshotId = currentSnapshotInfo == null ?
snapshotChainManager.getLatestGlobalSnapshotId() :
SnapshotUtils.getPreviousSnapshotId(currentSnapshotInfo, snapshotChainManager);
- Map<UUID, Pair<Long, Long>> exclusiveSizeMap = Maps.newConcurrentMap();
-
- CompletableFuture<Boolean> processedAllDeletedDirs = CompletableFuture.completedFuture(true);
final int parallelThreads = numberOfParallelThreadsPerStore.get();
+ CountDownLatch snapshotDbHandlesClosed = currentSnapshotInfo == null ? null :
Review Comment:
I added an AOS regression with three workers. One retains a snapshot DB read lock while another waits for unflushed transaction capacity. The scanning worker can still finish and release its handle, allowing both requests and the flush to complete. The diagram needs an additional dependency preventing worker B from finishing to form a deadlock. AOS therefore retains independent per-worker handle closure before submission.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -824,12 +920,25 @@ public BackgroundTaskResult call() {
} else if (!isPreviousPurgeTransactionFlushed()) {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot = snapInfo == null ? null :
- omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(), snapInfo.getBucketName(),
- snapInfo.getName())) {
- KeyManager keyManager = snapInfo == null ? getOzoneManager().getKeyManager()
- : omSnapshot.get().getKeyManager();
- processDeletedDirsForStore(snapInfo, keyManager, run, pathLimitPerTask);
+ boolean snapshotProcessingLockAcquired = false;
Review Comment:
AOS did bypass the snapshot-task lock and share its deletion pool. The revision uses one coordinator for both, so another store cannot open snapshot handles until the current store’s workers finish. Workers within each store remain parallel. The regression checks that the next coordinator task stays queued during submission. The specific A/B/purge diagram still needs a dependency stopping B’s scan to establish deadlock.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org