This is an automated email from the ASF dual-hosted git repository.
neilj 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 5dd389b7cb HDDS-5866. Discrepancy in Trash directory in ofs vs o3fs.
(#3906)
5dd389b7cb is described below
commit 5dd389b7cb9d6dd8ba425c5c4fd8678a2bc5c7b1
Author: Sadanand Shenoy <[email protected]>
AuthorDate: Thu Nov 3 20:59:15 2022 +0530
HDDS-5866. Discrepancy in Trash directory in ofs vs o3fs. (#3906)
* HDDS-5866.Discrepancy in Trash directory in ofs vs o3fs
* add new line
* fix test and set default trash policy
* fix test
* rebase and address comment
---
.../org/apache/hadoop/ozone/OzoneConfigKeys.java | 5 +
.../common/src/main/resources/ozone-default.xml | 10 ++
.../hadoop/fs/ozone/TestRootedOzoneFileSystem.java | 57 +++--------
.../hadoop/ozone/shell/TestOzoneShellHA.java | 19 ++--
.../apache/hadoop/ozone/om/TrashPolicyOzone.java | 105 ++++++++++++++++++++-
5 files changed, 140 insertions(+), 56 deletions(-)
diff --git
a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
index 733cce1ec2..80c531550f 100644
---
a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
+++
b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
@@ -533,6 +533,11 @@ public final class OzoneConfigKeys {
public static final String
OZONE_FS_LISTING_PAGE_SIZE_MAX = "ozone.fs.listing.page.size.max";
+
+ public static final String FS_TRASH_CLASSNAME = "fs.trash.classname";
+ public static final String FS_TRASH_CLASSNAME_DEFAULT =
+ "org.apache.hadoop.ozone.om.TrashPolicyOzone";
+
/**
* There is no need to instantiate this class.
*/
diff --git a/hadoop-hdds/common/src/main/resources/ozone-default.xml
b/hadoop-hdds/common/src/main/resources/ozone-default.xml
index 7b9132b2aa..2176f0a8df 100644
--- a/hadoop-hdds/common/src/main/resources/ozone-default.xml
+++ b/hadoop-hdds/common/src/main/resources/ozone-default.xml
@@ -2274,6 +2274,16 @@
Iterate batch size of delete when use BasicOzoneFileSystem.
</description>
</property>
+
+ <property>
+ <name>fs.trash.classname</name>
+ <value>org.apache.hadoop.ozone.om.TrashPolicyOzone</value>
+ <tag>OZONE, OZONEFS, CLIENT</tag>
+ <description>
+ Trash Policy to be used.
+ </description>
+ </property>
+
<property>
<name>ozone.manager.db.checkpoint.transfer.bandwidthPerSec</name>
<value>0</value>
diff --git
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
index 96ac47f065..437e30b8d8 100644
---
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
+++
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java
@@ -64,12 +64,10 @@ import org.apache.hadoop.ozone.security.acl.OzoneAclConfig;
import org.apache.hadoop.security.UserGroupInformation;
import org.apache.ozone.test.GenericTestUtils;
import org.apache.ozone.test.LambdaTestUtils;
-import org.apache.ozone.test.tag.Flaky;
import org.junit.After;
import org.junit.Assert;
import org.junit.Assume;
import org.junit.Before;
-import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.Timeout;
@@ -255,8 +253,6 @@ public class TestRootedOzoneFileSystem {
conf.setInt(OZONE_FS_ITERATE_BATCH_SIZE, 5);
// fs.ofs.impl would be loaded from META-INF, no need to manually set it
fs = FileSystem.get(conf);
- conf.setClass("fs.trash.classname", TrashPolicyOzone.class,
- TrashPolicy.class);
trash = new Trash(conf);
ofs = (RootedOzoneFileSystem) fs;
adapter = (BasicRootedOzoneClientAdapterImpl) ofs.getAdapter();
@@ -1523,39 +1519,6 @@ public class TestRootedOzoneFileSystem {
Assert.assertTrue(volume1.setOwner(prevOwner));
}
- /**
- * Check that files are moved to trash since it is enabled by
- * fs.rename(src, dst, options).
- */
- @Test
- @Flaky({"HDDS-5819", "HDDS-6451"})
- public void testRenameToTrashEnabled() throws IOException {
- // Create a file
- String testKeyName = "testKey2";
- Path path = new Path(bucketPath, testKeyName);
- try (FSDataOutputStream stream = fs.create(path)) {
- stream.write(1);
- }
-
- // Call moveToTrash. We can't call protected fs.rename() directly
- trash.moveToTrash(path);
-
- // Construct paths
- String username = UserGroupInformation.getCurrentUser().getShortUserName();
- Path trashRoot = new Path(bucketPath, TRASH_PREFIX);
- Path userTrash = new Path(trashRoot, username);
- Path userTrashCurrent = new Path(userTrash, "Current");
- String key = path.toString().substring(1);
- Path trashPath = new Path(userTrashCurrent, key);
- // Trash Current directory should still have been created.
- Assert.assertTrue(ofs.exists(userTrashCurrent));
- // Check under trash, the key should be present
- Assert.assertTrue(ofs.exists(trashPath));
-
- // Cleanup
- ofs.delete(trashRoot, true);
- }
-
@Test
public void testFileDelete() throws Exception {
Path grandparent = new Path(bucketPath, "testBatchDelete");
@@ -1600,7 +1563,6 @@ public class TestRootedOzoneFileSystem {
* 3.Create a second Key in different bucket and verify deletion.
* @throws Exception
*/
- @Ignore
@Test
public void testTrash() throws Exception {
String testKeyName = "keyToBeDeleted";
@@ -1635,21 +1597,26 @@ public class TestRootedOzoneFileSystem {
long prevNumTrashAtomicDirRenames = getOMMetrics()
.getNumTrashAtomicDirRenames();
- // Call moveToTrash. We can't call protected fs.rename() directly
- trash.moveToTrash(keyPath1);
- // for key in second bucket
- trash.moveToTrash(keyPath2);
-
// Construct paths for first key
String username = UserGroupInformation.getCurrentUser().getShortUserName();
Path trashRoot = new Path(bucketPath, TRASH_PREFIX);
Path userTrash = new Path(trashRoot, username);
- Path trashPath = getTrashKeyPath(keyPath1, userTrash);
+ Path userTrashCurrent = new Path(userTrash, "Current");
+ Path trashPath = new Path(userTrashCurrent, testKeyName);
// Construct paths for second key in different bucket
Path trashRoot2 = new Path(bucketPath2, TRASH_PREFIX);
Path userTrash2 = new Path(trashRoot2, username);
- Path trashPath2 = getTrashKeyPath(keyPath2, userTrash2);
+ Path trashPath2 = new Path(userTrashCurrent, testKeyName + "1");
+
+ // Call moveToTrash. We can't call protected fs.rename() directly
+ trash.moveToTrash(keyPath1);
+ // for key in second bucket
+ trash.moveToTrash(keyPath2);
+
+ // key should either be present in Current or checkpointDir
+ Assert.assertTrue(ofs.exists(trashPath)
+ || ofs.listStatus(ofs.listStatus(userTrash)[0].getPath()).length > 0);
// Wait until the TrashEmptier purges the keys
diff --git
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java
index 81a01190d1..88f387afee 100644
---
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java
+++
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java
@@ -581,19 +581,23 @@ public class TestOzoneShellHA {
getClientConfForOFS(hostPrefix, cluster.getConf());
OzoneFsShell shell = new OzoneFsShell(clientConf);
FileSystem fs = FileSystem.get(clientConf);
- final String strDir1 = hostPrefix + "/volumed2t/bucket1/dir1";
+ String ofsPrefix = hostPrefix + "/volumed2t/bucket1";
+ String dir1 = "/dir1";
+ final String strDir1 = ofsPrefix + dir1;
// Note: CURRENT is also privately defined in TrashPolicyDefault
final Path trashCurrent = new Path("Current");
final String strKey1 = strDir1 + "/key1";
final Path pathKey1 = new Path(strKey1);
- final Path trashPathKey1 = Path.mergePaths(new Path(
- new OFSPath(strKey1).getTrashRoot(), trashCurrent), pathKey1);
+ final Path trashPathKey1 = Path.mergePaths(
+ new Path(new OFSPath(strKey1).getTrashRoot(), trashCurrent),
+ new Path(dir1, "key1"));
final String strKey2 = strDir1 + "/key2";
final Path pathKey2 = new Path(strKey2);
- final Path trashPathKey2 = Path.mergePaths(new Path(
- new OFSPath(strKey2).getTrashRoot(), trashCurrent), pathKey2);
+ final Path trashPathKey2 = Path.mergePaths(
+ new Path(new OFSPath(strKey2).getTrashRoot(), trashCurrent),
+ new Path(dir1, "key2"));
int res;
try {
@@ -659,7 +663,8 @@ public class TestOzoneShellHA {
// create volume: vol1 with bucket: bucket1
final String testVolBucket = "/vol1/bucket1";
- final String testKey = testVolBucket + "/key1";
+ String keyName = "/key1";
+ final String testKey = testVolBucket + keyName;
final String[] volBucketArgs = new String[] {"-mkdir", "-p",
testVolBucket};
final String[] keyArgs = new String[] {"-touch", testKey};
@@ -690,7 +695,7 @@ public class TestOzoneShellHA {
testVolBucket + "/.Trash"};
final Path trashPathKey1 = Path.mergePaths(new Path(
new OFSPath(testKey).getTrashRoot(), new Path("Current")),
- new Path(testKey));
+ new Path(keyName));
FileSystem fs = FileSystem.get(clientConf);
try {
diff --git
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
index e1138afc8e..da42265438 100644
---
a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
@@ -38,6 +38,7 @@ import org.apache.hadoop.fs.permission.FsAction;
import org.apache.hadoop.fs.permission.FsPermission;
import org.apache.hadoop.fs.InvalidPathException;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.conf.OMClientConfig;
import org.apache.hadoop.ozone.om.helpers.OzoneFSUtils;
import org.apache.hadoop.ozone.OFSPath;
@@ -123,7 +124,100 @@ public class TrashPolicyOzone extends TrashPolicyDefault {
@Override
public boolean moveToTrash(Path path) throws IOException {
- this.fs.getFileStatus(path);
+ if (validatePath(path)) {
+ if (!isEnabled()) {
+ return false;
+ }
+
+ if (!path.isAbsolute()) { // make path absolute
+ path = new Path(fs.getWorkingDirectory(), path);
+ }
+
+ // check that path exists
+ fs.getFileStatus(path);
+ String qpath = fs.makeQualified(path).toString();
+
+ Path trashRoot = fs.getTrashRoot(path);
+ Path trashCurrent = new Path(trashRoot, CURRENT);
+ if (qpath.startsWith(trashRoot.toString())) {
+ return false; // already in trash
+ }
+
+ if (trashRoot.getParent().toString().startsWith(qpath)) {
+ throw new IOException("Cannot move \"" + path
+ + "\" to the trash, as it contains the trash");
+ }
+
+ Path trashPath;
+ Path baseTrashPath;
+ if (fs.getUri().getScheme().equals(OzoneConsts.OZONE_OFS_URI_SCHEME)) {
+ OFSPath ofsPath = new OFSPath(path);
+ // trimming volume and bucket in order to be compatible with o3fs
+ // Also including volume and bucket name in the path is redundant as
+ // the key is already in a particular volume and bucket.
+ Path trimmedVolumeAndBucket =
+ new Path(OzoneConsts.OZONE_URI_DELIMITER
+ + ofsPath.getKeyName());
+ trashPath = makeTrashRelativePath(trashCurrent,
trimmedVolumeAndBucket);
+ baseTrashPath = makeTrashRelativePath(trashCurrent,
+ trimmedVolumeAndBucket.getParent());
+ } else {
+ trashPath = makeTrashRelativePath(trashCurrent, path);
+ baseTrashPath = makeTrashRelativePath(trashCurrent, path.getParent());
+ }
+
+ IOException cause = null;
+
+ // try twice, in case checkpoint between the mkdirs() & rename()
+ for (int i = 0; i < 2; i++) {
+ try {
+ if (!fs.mkdirs(baseTrashPath, PERMISSION)) { // create current
+ LOG.warn("Can't create(mkdir) trash directory: " + baseTrashPath);
+ return false;
+ }
+ } catch (FileAlreadyExistsException e) {
+ // find the path which is not a directory, and modify baseTrashPath
+ // & trashPath, then mkdirs
+ Path existsFilePath = baseTrashPath;
+ while (!fs.exists(existsFilePath)) {
+ existsFilePath = existsFilePath.getParent();
+ }
+ baseTrashPath = new Path(baseTrashPath.toString()
+ .replace(existsFilePath.toString(),
+ existsFilePath.toString() + Time.now()));
+ trashPath = new Path(baseTrashPath, trashPath.getName());
+ // retry, ignore current failure
+ --i;
+ continue;
+ } catch (IOException e) {
+ LOG.warn("Can't create trash directory: " + baseTrashPath, e);
+ cause = e;
+ break;
+ }
+ try {
+ // if the target path in Trash already exists, then append with
+ // a current time in millisecs.
+ String orig = trashPath.toString();
+
+ while (fs.exists(trashPath)) {
+ trashPath = new Path(orig + Time.now());
+ }
+
+ // move to current trash
+ fs.rename(path, trashPath);
+ LOG.info("Moved: '" + path + "' to trash at: " + trashPath);
+ return true;
+ } catch (IOException e) {
+ cause = e;
+ }
+ }
+ throw (IOException) new IOException("Failed to move to trash: " + path)
+ .initCause(cause);
+ }
+ return false;
+ }
+
+ private boolean validatePath(Path path) throws IOException {
String key = path.toUri().getPath();
// Check to see if bucket is path item to be deleted.
// Cannot moveToTrash if bucket is deleted,
@@ -146,11 +240,14 @@ public class TrashPolicyOzone extends TrashPolicyDefault {
// first condition tests when length key is <= length trash
// and second when length key > length trash
if ((key.contains(this.fs.TRASH_PREFIX)) && (trashRootKey.startsWith(key))
- || key.startsWith(trashRootKey)) {
+ || key.startsWith(trashRootKey)) {
return false;
- } else {
- return super.moveToTrash(path);
}
+ return true;
+ }
+
+ private Path makeTrashRelativePath(Path basePath, Path rmFilePath) {
+ return Path.mergePaths(basePath, rmFilePath);
}
protected class Emptier implements Runnable {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
