HDDS-4043. allow deletion from Trash directory without -skipTrash option

[neils-dev]

What changes were proposed in this pull request?

To have FsShell rm (eg. bin/ozone fs -rm o3fs://bucket.volume/key) commands delete items from the filesystem Trash without need of the -skipTrash flag, changes are made to the ozone filesystem specific TrashPolicy. Implemented through overriding the TrashPolicy moveTrash method for TrashPolicyOzone.

Here if the item to be deleted with the FsShell rm command is located in the key root trash directory, it is deleted from the filesystem. Before this patch, items removed through the rm command were moved to the root Trash directory when trash was enabled. Patch enacts similar behavior as that of FsShell rm command with the -skipTrash flag specified. Behavior with the -skipTrash flag enabled is unaffected.

Necessary for this patch is configuring the hadoop filesystem to use the ozone implementation specific TrashPolicyOzone though the 'core-site.xml' as in :
<property> <name>fs.trash.classname</name> <value>org.apache.hadoop.ozone.om.TrashPolicyOzone</value> </property>

What is the link to the Apache JIRA

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

How was this patch tested?

Patch was tested through both integration tests and manual tests.

Integration test implemented in ozone shell integration test : TestOzoneShellHA.java. Tested through hadoop-ozone/integration_test$ mvn -Dtest=TestOzoneShellHA#testDeleteTrashNoSkipTrash test

---

Test set: org.apache.hadoop.ozone.shell.TestOzoneShellHA

Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 28.099 s - in org.apache.hadoop.ozone.shell.TestOzoneShellHA

Manual test through docker cluster deployment:

before patch ->

1. configure core-site.xml to use TrashPolicyDefault <property> <name>fs.trash.classname</name> <value>org.apache.hadoop.ozone.om.TrashPolicyDefault</value> </property> OR leave unchanged (default behavior)
   enable Trash
   <property> <name>fs.trash.interval</name> <value>60</value> </property>
2. run shell commands to create volume, bucket and key (eg. /vol1/bucket1/key1)
3. run shell command to delete key (eg. bin/ozone fs -rm -R o3fs://bucket1.vol1/key1)
4. run shell command to delete trash (eg. bin/ozone fs -rm -R o3fs://bucket1.vol1/.Trash)
   Trash directory remains.
   

patch ->

1. configure core-site.xml to use TrashPolicyOzone <property> <name>fs.trash.classname</name> <value>org.apache.hadoop.ozone.om.TrashPolicyOzone</value> </property>
   enable Trash
   <property> <name>fs.trash.interval</name> <value>60</value> </property>
2. run shell commands to create volume, bucket and key (eg. /vol1/bucket1/key1)
3. run shell command to delete key (eg. bin/ozone fs -rm -R o3fs://bucket1.vol1/key1)
4. run shell command to delete trash (eg. bin/ozone fs -rm -R o3fs://bucket1.vol1/.Trash)
   Trash directory is deleted.

[sadanand48]

Thanks @neils-dev for the good work. I have left a few minor comments . Apart from that patch looks good to me.

[sadanand48]

Please remove trashCurrent variable as it is unused

[neils-dev]

Removed unused variable.

[sadanand48]

I think only key.startsWith(trashRootKey) check would suffice. Do we need the OR condition?

[neils-dev]

Thanks for asking to clarify. Added comments in code to explain. In process also improved conditional to satisfy an edge condition where key = .Tra (and the trash prefix is .Trash that would cause an unintentional action, by deleting and not moving to trash).

    // 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)) {

[sadanand48]

Thanks for the explaining it in the comment . Although , when length of key < length of trashroot , the key would not be inside .Trash directory.
Suppose key = vol/buck/.Trash/user/randomKey ,the trashroot here would be vol/buck/.Trash
So length of key is always greater than or equal to the length of trashroot and first condition would not be required.

[neils-dev]

Thanks @sadanand48, yes for what you described that is true. The trashroot is derived from the bucket under the key, trashroot=/vol/bucket/.Trash/username. One case where length key < length trashroot can be when key = /vol/bucket/.Trash; here trashroot is /vol/bucket/.Trash/username.

[sadanand48]

Thanks for clarifying this @neils-dev . I forgot to consider the username in trashroot. It makes sense now . Thanks

[neils-dev]

Thanks @sadanand48 for reviewing the PR and the great comments! Latest commit with changes for comments.

[sadanand48]

@neils-dev Please use getClientConfForOFS(hostPrefix,conf) for initialising clientConf here. The integration test is failing because of this

 // Test delete from Trash directory removes item from filesystem

    // setup configuration to use TrashPolicyOzone
    // (default is TrashPolicyDefault)
    final String hostPrefix = OZONE_OFS_URI_SCHEME + "://" + omServiceId;
    OzoneConfiguration clientConf = getClientConfForOFS(hostPrefix,conf);
    clientConf.setInt(FS_TRASH_INTERVAL_KEY, 60);
    clientConf.setClass("fs.trash.classname", TrashPolicyOzone.class,
                        TrashPolicy.class);
    OzoneFsShell shell = new OzoneFsShell(clientConf);

[neils-dev]

Thanks for the comment @sadanand48. Commit to set configuration in function setting TrashPolicyOzone after getClientConfForOFS as suggested.

[sadanand48]

Thanks @neils-dev for making the change . Sorry , I wasn't clear . I meant to use the same URI as that of testDeleteToTrashOrSkipTrash i.e to include serviceId as part of the URI. Since this is HA setup ,its required to pass the serviceID. Please use the same URI as that of testDeleteToTrashOrSkipTrash i.e

final String hostPrefix = OZONE_OFS_URI_SCHEME + "://" + omServiceId;
OzoneConfiguration clientConf = getClientConfForOFS(hostPrefix,conf);

instead of
OzoneConfiguration clientConf = getClientConfForOzoneTrashPolicy("ofs://localhost", conf);

I tried running the individual test in IDE and it's failing. The test only passes when the entire test file is run which is why the integration test passed.

[neils-dev]

Thanks for your comment @sadanand48 . Commit a change for the service id as suggested. Thanks for following up. BTW, the host set to localhost, as was previously used for the clientconf should not be a problem with the integration test. It was tested both individually hadoop-ozone/integration-test$ mvn -Dtest=TestOzoneShellHA#testDeleteTrashNoSkipTrash test and when the entire suite is run, hadoop-ozone/integration-test$ mvn -Dtest=TestOzoneShellHA test. See attached image.

hadoop-ozone/integration-test$ mvn -Dtest=TestOzoneShellHA#testDeleteTrashNoSkipTrash test

[sadanand48]

Thanks @neils-dev for addressing comments. +1 . LGTM

[elek]

Thanks the patch @neils-dev. It looks good to me, but I tried to run the Unit test locally before the commit.

When I try to debug it the unit test is frequently failing:

java.lang.AssertionError: 
Expected :0
Actual   :1
<Click to see difference>


	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
	at org.apache.hadoop.ozone.shell.TestOzoneShellHA.testDeleteTrashNoSkipTrash(TestOzoneShellHA.java:635)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:288)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:282)
	at java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:266)
	at java.util.concurrent.FutureTask.run(FutureTask.java)
	at java.lang.Thread.run(Thread.java:748)

So I tried to run the unit with IntelliJ "repeat until fail" functionality, but it is always failed at the second time.

Do you have the same behavior? Can you run it multiple times without any error?

I would like to be sure that it's not an intermittent test.

[neils-dev]

Thanks for looking into potential problems with integration test @elek. I can confirm the error you see on your local test environment, however the messages I see associated with running the test 'repeat until fail' are timeout errors - 2021-05-07 15:51:56,695 [Time-limited test] ERROR ha.OMFailoverProxyProvider (OMFailoverProxyProvider.java:getRetryAction(294)) - Failed to connect to OMs: [nodeId=omNode-3,nodeAddress=127.0.0.1:13660, nodeId=omNode-1,nodeAddress=127.0.0.1:13648, nodeId=omNode-2,nodeAddress=127.0.0.1:13654]. Attempted 15 failovers. Got following exceptions during retries: ; followed by the assertion error you posted.

In addition I tried the same test setup with "repeat until failure" option with Intellij on the org.apache.hadoop.ozone.shell.TestOzoneShellHA.testDeleteToTrashOrSkipTrash with same error.

It does not appear to be intermittent, rather a problem with running the tests continuously. It may be from parts of the test initialized globally for org.apache.hadoop.ozone.shell.TestOzoneShellHA (@before, @after, @BeforeClass ...), hence are not reinitialized on each invocation of a specific test such as testDeleteToTrashOrSkipTrash. I will look further into this and update.

[neils-dev]

The problem indeed involves running the TestOzoneShellHA trash tests repetitively continuously. The test cluster is initialized at the start of the suite of tests with the Om rpc server and Om rpc client in sync, however when the test is repeated as in this case, the rpc client becomes out of sync and uses the wrong port to access the Om rpc server - occurs with shell rpc submitRequest calls. Problem is identified. Will investigate further.

[neils-dev]

Recent commit contains fix for problem running trash service integration tests consecutively (repeat until failure). The OmTransport RPC connection between the shell and om was not closed at end of test; improper cleanup prior to start of next test. Fixed with calling the close() method of the filesystem class implementation.

[sadanand48]

Thanks @neils-dev for identifying the issue. @elek Can you please take a look and merge the change?

[elek]

Thanks for the review @sadanand48 and the patch @neils-dev. Merging it now.