This is an automated email from the ASF dual-hosted git repository.
siyao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new e8e34c2a91 HDDS-7914. [Snapshot] Block FS API access to deleted
(non-active) snapshots (#4419)
e8e34c2a91 is described below
commit e8e34c2a9170462adff57611caa09cfda55fc8cc
Author: Siyao Meng <[email protected]>
AuthorDate: Wed Mar 29 15:26:22 2023 -0700
HDDS-7914. [Snapshot] Block FS API access to deleted (non-active) snapshots
(#4419)
---
.../hadoop/ozone/om/TestOmSnapshotFileSystem.java | 70 +++++++++++++++++++++-
.../org/apache/hadoop/ozone/om/OmSnapshot.java | 2 +-
.../apache/hadoop/ozone/om/OmSnapshotManager.java | 52 ++++++++++++++++
.../request/snapshot/OMSnapshotDeleteRequest.java | 5 ++
.../hadoop/ozone/om/TestOmSnapshotManager.java | 3 +-
.../snapshot/TestOMSnapshotDeleteRequest.java | 7 +++
6 files changed, 134 insertions(+), 5 deletions(-)
diff --git
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotFileSystem.java
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotFileSystem.java
index 1e822d6159..ba5c82bdc3 100644
---
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotFileSystem.java
+++
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotFileSystem.java
@@ -39,16 +39,19 @@ import org.apache.hadoop.ozone.client.OzoneKey;
import org.apache.hadoop.ozone.client.OzoneVolume;
import org.apache.hadoop.ozone.client.io.OzoneInputStream;
import org.apache.hadoop.ozone.client.io.OzoneOutputStream;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
import org.apache.hadoop.ozone.om.helpers.BucketLayout;
import org.apache.hadoop.ozone.om.helpers.OmKeyArgs;
import org.apache.hadoop.ozone.om.helpers.OpenKeySession;
import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
import org.apache.hadoop.ozone.om.protocol.OzoneManagerProtocol;
import org.apache.ozone.test.GenericTestUtils;
+import org.apache.ozone.test.LambdaTestUtils;
import org.junit.After;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
+import org.junit.jupiter.api.Assertions;
import org.junit.rules.Timeout;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -134,11 +137,13 @@ public class TestOmSnapshotFileSystem {
init();
}
}
+
private static void setConfig(BucketLayout newBucketLayout,
boolean newEnableFileSystemPaths) {
TestOmSnapshotFileSystem.enabledFileSystemPaths = newEnableFileSystemPaths;
TestOmSnapshotFileSystem.bucketLayout = newBucketLayout;
}
+
/**
* Create a MiniDFSCluster for testing.
*/
@@ -367,6 +372,60 @@ public class TestOmSnapshotFileSystem {
keyPrefix = s;
}
+ @Test
+ public void testBlockSnapshotFSAccessAfterDeletion() throws Exception {
+ Path root = new Path("/");
+ Path dir = new Path(root, "/testListKeysBeforeAfterSnapshotDeletion");
+ Path key1 = new Path(dir, "key1");
+ Path key2 = new Path(dir, "key2");
+
+ // Create 2 keys
+ ContractTestUtils.touch(fs, key1);
+ ContractTestUtils.touch(fs, key2);
+
+ // Create a snapshot
+ String snapshotName = UUID.randomUUID().toString();
+ String snapshotKeyPrefix = createSnapshot(snapshotName);
+
+ // Can list keys in snapshot
+ Path snapshotRoot = new Path(snapshotKeyPrefix + root);
+ Path snapshotParent = new Path(snapshotKeyPrefix + dir);
+ // Check dir in snapshot
+ FileStatus[] fileStatuses = o3fs.listStatus(snapshotRoot);
+ Assertions.assertEquals(1, fileStatuses.length);
+ // List keys in dir in snapshot
+ fileStatuses = o3fs.listStatus(snapshotParent);
+ Assertions.assertEquals(2, fileStatuses.length);
+
+ // Check key metadata
+ Path snapshotKey1 = new Path(snapshotKeyPrefix + key1);
+ FileStatus fsActiveKey = o3fs.getFileStatus(key1);
+ FileStatus fsSnapshotKey = o3fs.getFileStatus(snapshotKey1);
+ Assert.assertEquals(fsActiveKey.getModificationTime(),
+ fsSnapshotKey.getModificationTime());
+
+ Path snapshotKey2 = new Path(snapshotKeyPrefix + key2);
+ fsActiveKey = o3fs.getFileStatus(key2);
+ fsSnapshotKey = o3fs.getFileStatus(snapshotKey2);
+ Assert.assertEquals(fsActiveKey.getModificationTime(),
+ fsSnapshotKey.getModificationTime());
+
+ // Delete the snapshot
+ deleteSnapshot(snapshotName);
+
+ // Can't access keys in snapshot anymore with FS API. Should throw
exception
+ final String errorMsg = "no longer active";
+ LambdaTestUtils.intercept(OMException.class, errorMsg,
+ () -> o3fs.listStatus(snapshotRoot));
+ LambdaTestUtils.intercept(OMException.class, errorMsg,
+ () -> o3fs.listStatus(snapshotParent));
+
+ LambdaTestUtils.intercept(OMException.class, errorMsg,
+ () -> o3fs.getFileStatus(snapshotKey1));
+ LambdaTestUtils.intercept(OMException.class, errorMsg,
+ () -> o3fs.getFileStatus(snapshotKey2));
+ }
+
@Test
// based on TestOzoneFileSystem:testListStatus
public void testListStatus() throws Exception {
@@ -524,7 +583,6 @@ public class TestOmSnapshotFileSystem {
writeClient.commitKey(keyArgs, session.getId());
}
-
/**
* Tests listStatus operation on root directory.
*/
@@ -637,9 +695,13 @@ public class TestOmSnapshotFileSystem {
private String createSnapshot()
throws IOException, InterruptedException, TimeoutException {
+ return createSnapshot(UUID.randomUUID().toString());
+ }
+
+ private String createSnapshot(String snapshotName)
+ throws IOException, InterruptedException, TimeoutException {
// create snapshot
- String snapshotName = UUID.randomUUID().toString();
writeClient.createSnapshot(volumeName, bucketName, snapshotName);
// wait till the snapshot directory exists
@@ -654,4 +716,8 @@ public class TestOmSnapshotFileSystem {
return OM_KEY_PREFIX + OmSnapshotManager.getSnapshotPrefix(snapshotName);
}
+
+ private void deleteSnapshot(String snapshotName) throws IOException {
+ writeClient.deleteSnapshot(volumeName, bucketName, snapshotName);
+ }
}
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java
index 3f3935e028..2a02e940a4 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java
@@ -67,6 +67,7 @@ public class OmSnapshot implements IOmMetadataReader,
Closeable {
private final String volumeName;
private final String bucketName;
private final String snapshotName;
+ // To access snapshot checkpoint DB metadata
private final OMMetadataManager omMetadataManager;
public OmSnapshot(KeyManager keyManager,
@@ -84,7 +85,6 @@ public class OmSnapshot implements IOmMetadataReader,
Closeable {
this.omMetadataManager = keyManager.getMetadataManager();
}
-
@Override
public OmKeyInfo lookupKey(OmKeyArgs args) throws IOException {
return denormalizeOmKeyInfo(omMetadataReader.lookupKey(
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java
index e05dc897dd..91fc4498c9 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java
@@ -51,6 +51,7 @@ import org.apache.hadoop.ozone.om.exceptions.OMException;
import org.apache.hadoop.ozone.om.helpers.RepeatedOmKeyInfo;
import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
import org.apache.hadoop.ozone.om.helpers.SnapshotInfo.SnapshotStatus;
+import org.apache.hadoop.ozone.om.service.SnapshotDeletingService;
import org.apache.hadoop.ozone.om.snapshot.SnapshotDiffManager;
import org.apache.hadoop.ozone.om.snapshot.SnapshotUtils;
import org.apache.hadoop.ozone.snapshot.SnapshotDiffResponse;
@@ -69,6 +70,7 @@ import static
org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX;
import static org.apache.hadoop.ozone.OzoneConsts.OM_SNAPSHOT_DIFF_DB_NAME;
import static org.apache.hadoop.ozone.OzoneConsts.OM_SNAPSHOT_INDICATOR;
import static
org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_SNAPSHOT_DIFF_DB_DIR;
+import static
org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.FILE_NOT_FOUND;
import static
org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.INVALID_KEY_NAME;
/**
@@ -174,6 +176,28 @@ public final class OmSnapshotManager implements
AutoCloseable {
// see if the snapshot exists
snapshotInfo = getSnapshotInfo(snapshotTableKey);
+ // Block snapshot from loading when it is no longer active e.g.
DELETED,
+ // unless this is called from SnapshotDeletingService.
+ // TODO: [SNAPSHOT] However, snapshotCache.get() from other requests
+ // (not from SDS) would be able to piggyback off of this because
+ // snapshot still in cache won't trigger loader again.
+ // This needs proper addressal in e.g. HDDS-7935
+ // by introducing another cache just for SDS.
+ // While the snapshotCache would host ACTIVE snapshots only.
+ if (!snapshotInfo.getSnapshotStatus().equals(
+ SnapshotStatus.SNAPSHOT_ACTIVE)) {
+ if (isCalledFromSnapshotDeletingService()) {
+ LOG.debug("Permitting {} to load snapshot {} in status: {}",
+ SnapshotDeletingService.class.getSimpleName(),
+ snapshotInfo.getTableKey(),
+ snapshotInfo.getSnapshotStatus());
+ } else {
+ throw new OMException("Unable to load snapshot. " +
+ "Snapshot with table key '" + snapshotTableKey +
+ "' is no longer active", FILE_NOT_FOUND);
+ }
+ }
+
CacheValue<SnapshotInfo> cacheValue =
ozoneManager.getMetadataManager().getSnapshotInfoTable()
.getCacheValue(new CacheKey<>(snapshotTableKey));
@@ -214,6 +238,8 @@ public final class OmSnapshotManager implements
AutoCloseable {
= notification -> {
try {
// close snapshot's rocksdb on eviction
+ LOG.debug("Closing snapshot: {}", notification.getKey());
+ // TODO: [SNAPSHOT] HDDS-7935.Close only when refcount reaches
zero?
notification.getValue().close();
} catch (IOException e) {
LOG.error("Failed to close snapshot: {} {}",
@@ -232,6 +258,32 @@ public final class OmSnapshotManager implements
AutoCloseable {
columnFamilyOptions);
}
+ /**
+ * Helper method to check whether the loader is called from
+ * SnapshotDeletingTask (return true) or not (return false).
+ */
+ private boolean isCalledFromSnapshotDeletingService() {
+
+ StackTraceElement[] stackTrace = Thread.currentThread().getStackTrace();
+ for (StackTraceElement elem : stackTrace) {
+ // Allow as long as loader is called from SDS. e.g. SnapshotDeletingTask
+ if (elem.getClassName().startsWith(
+ SnapshotDeletingService.class.getName())) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ /**
+ * Get snapshot instance LRU cache.
+ * @return LoadingCache
+ */
+ public LoadingCache<String, OmSnapshot> getSnapshotCache() {
+ return snapshotCache;
+ }
+
/**
* Creates snapshot checkpoint that corresponds to snapshotInfo.
* @param omMetadataManager the metadata manager
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotDeleteRequest.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotDeleteRequest.java
index b2684d9cd1..7af3f1486f 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotDeleteRequest.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotDeleteRequest.java
@@ -184,6 +184,11 @@ public class OMSnapshotDeleteRequest extends
OMClientRequest {
omClientResponse = new OMSnapshotDeleteResponse(
omResponse.build(), tableKey, snapshotInfo);
+ // Evict the snapshot entry from cache, and close the snapshot DB
+ // Nothing happens if the key doesn't exist in cache (snapshot not
loaded)
+ ozoneManager.getOmSnapshotManager().getSnapshotCache()
+ .invalidate(tableKey);
+
} catch (IOException ex) {
exception = ex;
omClientResponse = new OMSnapshotDeleteResponse(
diff --git
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java
index 7da918de1f..534524c778 100644
---
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java
+++
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java
@@ -97,7 +97,7 @@ public class TestOmSnapshotManager {
HddsWhiteboxTestUtils.setInternalState(
firstSnapshot.getMetadataManager(), "store", firstSnapshotStore);
- // create second snapshot checkpoint (which will be used for eviction)
+ // create second snapshot checkpoint (which will be used for eviction)
OmSnapshotManager.createOmSnapshotCheckpoint(om.getMetadataManager(),
second);
@@ -109,7 +109,6 @@ public class TestOmSnapshotManager {
.checkForSnapshot(second.getVolumeName(),
second.getBucketName(), getSnapshotPrefix(second.getName()));
-
// confirm store was closed
verify(firstSnapshotStore, timeout(3000).times(1)).close();
}
diff --git
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java
index c872312c1b..0012e9494f 100644
---
a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java
+++
b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java
@@ -20,6 +20,7 @@
package org.apache.hadoop.ozone.om.request.snapshot;
+import com.google.common.cache.LoadingCache;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.utils.db.cache.CacheKey;
import org.apache.hadoop.hdds.utils.db.cache.CacheValue;
@@ -28,6 +29,7 @@ import org.apache.hadoop.ozone.audit.AuditMessage;
import org.apache.hadoop.ozone.om.OMConfigKeys;
import org.apache.hadoop.ozone.om.OMMetrics;
import org.apache.hadoop.ozone.om.OmMetadataManagerImpl;
+import org.apache.hadoop.ozone.om.OmSnapshotManager;
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.om.exceptions.OMException;
import org.apache.hadoop.ozone.om.helpers.SnapshotInfo;
@@ -98,6 +100,11 @@ public class TestOMSnapshotDeleteRequest {
when(ozoneManager.getAuditLogger()).thenReturn(auditLogger);
Mockito.doNothing().when(auditLogger).logWrite(any(AuditMessage.class));
+ OmSnapshotManager omSnapshotManager = mock(OmSnapshotManager.class);
+ when(omSnapshotManager.getSnapshotCache())
+ .thenReturn(mock(LoadingCache.class));
+ when(ozoneManager.getOmSnapshotManager()).thenReturn(omSnapshotManager);
+
volumeName = UUID.randomUUID().toString();
bucketName = UUID.randomUUID().toString();
snapshotName = UUID.randomUUID().toString();
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
