HDDS-7828. Make Ozone fs rm symbolic links command support posix behaviour #4246

neils-dev · 2023-02-04T05:33:36Z

What changes were proposed in this pull request?

To make ofs symlink delete behavior similar to posix for ozone fs -rm command requests. Specifically to have,
i. $ ozonefs -rm -skipTrash ofs://<omserviceid>/volume/<linked bucket symlink>
delete symlink
ii. $ ozonefs -rm --R -f skipTrash ofs://<omserviceid>/volume/<linked bucket symlink>/
recursively delete the contents of the source bucket leaving the symlink and source bucket intact.

Currently, i.) results in an error indicating cannot remove directory and ii.) results in deleting the symlink.

To add posix like symlink -rm command behavior an ozone specific -rm command handler, OzoneDelete, is implemented and binded with the fs shell commands.

What is the link to the Apache JIRA

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

How was this patch tested?

Unit tests, for -rm command handler. Integration tests for symlink delete behavior supporting -rm removing symlink and -rm -R with trailing slash to remove symlink resolved contents.

Unit tests:
TestOzoneFsShell#testOzoneFsShellRegisterDeleteCmd
Integration tests:
TestRootedOzoneFileSystem.#testSymlinkPosixDelete
TestRootedOzoneFileSystem.#testSymlinkList (added test when testing symlink rm behavior)

Manual tests:
docker dev cluster -

# setup src bucket and linked bucket with key
bash-4.2$ ozone sh volume create /vol1
bash-4.2$ ozone sh bucket create /vol1/abc
bash-4.2$ ozone sh bucket link /vol1/abc /s3v/abc
bash-4.2$ ozone sh key put /vol1/abc/key1 ./README.md

# delete just symlink with -rm command
bash-4.2$ ozone fs -rm -f -skipTrash ofs://om/s3v/abc
# check with -ls that no linked bucket
bash-4.2$ ozone fs -ls ofs://om/s3v
# create symlink again
bash-4.2$ ozone sh bucket link /vol1/abc /s3v/abc
# delete contents of src bucket with -rm with trailing slash
bash-4.2$ ozone fs -rm -R -f -skipTrash ofs://om/s3v/abc/
# check symlink exists
bash-4.2$ ozone fs -ls ofs://om/s3v

…posix symlink delete behavior.

…r listing symbolic link buckets and for deleting symobolic link buckets with ozonefs -ls and -rm commands.

adoroszlai

Thanks @neils-dev for working on this. The change in BasicRootedOzoneFileSystem looks good. I wonder if we can avoid duplicating the Delete handler from Hadoop by fixing the symlink argument being passed to FileStatusAdapter (currently hard-coded to null) for link buckets.

ozone/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java

Lines 1116 to 1119 in 0a377c6

    
           return new FileStatusAdapter(0L, 0L, path, true, (short)0, 0L, 
        
               ozoneBucket.getCreationTime().getEpochSecond() * 1000, 0L, 
        
               FsPermission.getDirDefault().toShort(), 
        
               owner, group, null, new BlockLocation[0],

neils-dev · 2023-02-07T03:38:51Z

Thanks @adoroszlai for your comment and for reviewing this.

I wonder if we can avoid duplicating the Delete handler from Hadoop by fixing the symlink argument being passed to FileStatusAdapter

Unfortunately, supporting filestatus symlink does not let us avoid binding in an Ozone Delete handler. The handler is used to set the delete behavior for -rm (rm symlink) and -rm with trailing slash (to remove contents of symlink dir, through the call to the FileSystem delete with trailing slash).

I too wanted to enable symlink in the filestatus, currently null, however it wasn't directly possible due to FileStatus on create only allowing an item to be either a directory or a symlink (asserts if both). This causes problems with the PathData that expects directories for recursing paths, used in listStatus and other commands.

hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFsShell.java

…HDDS-8077.

smengcl

overall looks good. nice test additions. minor comments inline.

smengcl · 2023-03-16T10:00:18Z

...zone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java

      // if link, don't delete contents
-      if (isBucketLink) {
+      if (isBucketLink && !handleTrailingSlash) {


Need to update the comment above accordingly

Thanks @smengcl for your review comments and for following up the PR. Latest commit to address review comments.

comment updated.

smengcl · 2023-03-16T10:00:25Z

...zone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java

      // Handle delete bucket
-      if (ofsPath.isBucket()) {
+      if (ofsPath.isBucket() && !handleTrailingSlash) {


comment updated.

smengcl · 2023-03-16T10:02:47Z

hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFsShell.java

    // commands, and then this method can be abstract
    if (this.getClass().equals(OzoneFsShell.class)) {
      factory.registerCommands(FsCommand.class);
+      // ozone delete rm command registration superscedes fs delete


nit

Suggested change

// ozone delete rm command registration superscedes fs delete

// ozone delete rm command registration. supercedes fs delete

smengcl · 2023-03-16T10:08:08Z

hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneDelete.java

+@InterfaceAudience.Private
+@InterfaceStability.Evolving
+
+public final class OzoneDelete {


What about OzoneFsDelete?

Suggested change

public final class OzoneDelete {

public final class OzoneFsDelete {

Yes, renamed. Updated the class name and all references to OzoneFsDelete

smengcl · 2023-03-16T10:24:18Z

hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneDelete.java

+      // TODO: if the user wants the trash to be used but there is any
+      // problem (ie. creating the trash dir, moving the item to be deleted,
+      // etc), then the path will just be deleted because moveToTrash returns
+      // false and it falls thru to fs.delete.  this doesn't seem right
+      if (moveToTrash(item) || !canBeSafelyDeleted(item)) {
+        return;
+      }


I'm trying to understand this TODO here.

I think in this case (where skipTrash == false but moving to trash has issues) we should just return error to the user. So that they either need to deal with that trash issue first (maybe some ACL issue), or set -skipTrash.

I don't think falling back to to skipTrash would be the intended user behavior, cause they could lose keys when they think those would be moved to the trash.

Also it looks like moveToTrash() is already throwing exception appropriately? Do we need extra handling here? If not we can just fix the comment.

Looked into the moveToTrash() comment.

just return error to the user. So that they either need to deal with that trash issue first (maybe some ACL issue), or set -skipTrash.

I suspect, like you that errors from creating the trash directory or renaming to trash will be handled properly with the exception thrown by trash - similar to

bash-4.2$ ozone fs -rm ofs://om/tmp/tmpfile rm: Failed to move to trash: ofs://om/tmp/tmpfile: User testuser2/scm@EXAMPLE.COM doesn't have DELETE permission to access key Volume:tmp Bucket:tmp Key:tmpfile. Consider using -skipTrash option

Here errors occurring from trash unable to create trash dir or renaming file are handled by exception handling and the file is not outright deleted by fs.delete. File remains intact leaving the user to resolve the issue through permission settings or -skipTrash.

I'm looking to update the comment to reflect this.

smengcl · 2023-03-16T10:45:30Z

...one/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java

+        Assert.assertEquals(0, res);
+
+        try {
+          objectStore.getVolume(destVolume).getBucket(destVolume);


typo?

Suggested change

objectStore.getVolume(destVolume).getBucket(destVolume);

objectStore.getVolume(destVolume).getBucket(srcBucket);

smengcl · 2023-03-16T10:48:23Z

...one/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystem.java

+        // test symlink -rm -R -f destVol/destBucket -> srcVol/srcBucket
+        // should delete only link
+        // run toolrunner ozone fs shell commands
+        linkPathStr = rootPath + destVolume + OZONE_URI_DELIMITER + srcBucket;
+        res = ToolRunner.run(shell, new String[]{"-rm", "-skipTrash",
+            "-f", "-R", linkPathStr});
+        Assert.assertEquals(0, res);
+
+        Assert.assertEquals(srcBucket, objectStore.getVolume(srcVolume)
+            .getBucket(srcBucket).getName());


Can also check /srcVolume/srcBucket/key existence

Added the test condition for

/srcVolume/srcBucket/key existence

thanks!

…ete, minor fixes and comment fixes to unit tests, minor comment fixes to BasicRootedOzoneFilesystem. Ammended comment on handling trash withing OzoneFsDelete command handler.

ChenSammi · 2023-04-18T10:09:55Z

@neils-dev , thanks for working on this. I will take a close look later. I tried 'rm -R ' in Linux on a symbolic link to a directory. It looks like that not only the files under the source directory are deleted, symbolic link is also deleted. Shall we follow this behavior too?

neils-dev · 2023-04-18T18:45:06Z

@ChenSammi thanks. With the example you mentioned, are you using a trailing slash with the command ie. rm -R -f path_to_symlink_with_slash/ with trailing slash?

'rm -R ' in Linux on a symbolic link to a directory. It looks like that not only the files under the source directory are deleted, symbolic link is also deleted.

That behavior looks to be supported - delete contents of src directory leaving symlink intact.
$ rm -R -f ~/work/tmp/tmp2/
$ ls -alt ~/work/tmp
lrwxrwxrwx 1 user user 7 Apr 18 12:13 tmp2 -> ../tmp1

ChenSammi · 2023-04-19T07:07:16Z

@neils-dev, I used the command "rm -R path-to-symbolic" without ending slash "/". So the path-to-symbolic is deleted.

You are correct, "rm -R path-to-symbolic/" will delete the content in source directory only.

Besides the "-rm" command, @neils-dev have you try the "-rmdir" command on the link bucket too? I'm not sure if it already works for link bucket now or not.

ChenSammi · 2023-04-19T08:12:36Z

hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneFsDelete.java

+  }
+
+  /** remove non-directory paths. */
+  public static class Rm extends FsCommand {


Can this Rm extend Delete.Rm? Except expandArgument and processPath are different, other parts of the implementation are the same as Delete.Rm.

Can this Rm extend Delete.Rm?

Thanks. Due to the interface, I found that none of the Delete class or any of the command handlers for the fs shells can be extended outside of the hadoop fs shell package.

@neils-dev We can put the subclass in the org.apache.hadoop.fs.shell package if necessary. Similar solution is employed for reusing other classes from Hadoop (e.g. for EC).

Can this Rm extend Delete.Rm?

After repackaging under the hadoop fs shell package, org.apache.hadoop.fs.shell, it is possible to extend Delete.Rm to a certain extent, however many of the properties and methods used in the subclass overriden methods are private in the base class. The private props and methods include all of the parsed command arguments (private variables in base class), and the garbage collection trash routines needed in the subclass override for processPath , The resulting subclass redefines those private methods to an extent and limits code reuse. It does not appear to be advantageous. This FsCommand does not lend itself to subclassing. Our ozone command handler OzoneFsDelete appears to be flexible and cleaner. We can open a jira to follow-up on looking into this further if it looks worth pursuing, Thoughts?

Thanks @neils-dev for checking. We'll have to live with duplication then.

@neils-dev , thanks for the checking. Then let's keep it.

The patch LGTM +1.

ChenSammi · 2023-04-26T13:12:43Z

@neils-dev , the failed test looks like relevant.

…links. Fixes CI build failure as a result of the recent symlink orphan node patch.

neils-dev added 4 commits July 26, 2021 18:41

Commit new workflow def for repeated test of HDDS-5358.

b432c98

Merge remote-tracking branch 'upstream/master'

bf18713

intial commit. Check CI workflow for patch for ofs to support pseudo-…

baf1050

…posix symlink delete behavior.

Added tests for ofs symbolic link handling, testing posix behavior fo…

176be30

…r listing symbolic link buckets and for deleting symobolic link buckets with ozonefs -ls and -rm commands.

adoroszlai reviewed Feb 4, 2023

View reviewed changes

adoroszlai requested review from jojochuang and smengcl February 4, 2023 11:00

neils-dev added the gr label Feb 15, 2023

adoroszlai reviewed Mar 5, 2023

View reviewed changes

hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFsShell.java Outdated Show resolved Hide resolved

Minor fix for checkstyle, enforcing newline at end of each file. See …

2c032a8

…HDDS-8077.

prashantpogde self-requested a review March 9, 2023 17:58

smengcl reviewed Mar 16, 2023

View reviewed changes

smengcl mentioned this pull request Mar 28, 2023

HDDS-7884. ofs: Recursive delete of path of symlink with empty source fails #4405

Merged

neils-dev added 2 commits April 14, 2023 12:08

Updates to address review comments. Renamed OzoneDelete to OzoneFsDel…

5a4302e

…ete, minor fixes and comment fixes to unit tests, minor comment fixes to BasicRootedOzoneFilesystem. Ammended comment on handling trash withing OzoneFsDelete command handler.

Merge branch 'master' into HDDS-7828

5f43c65

adoroszlai requested review from ChenSammi and smengcl April 17, 2023 10:30

ChenSammi reviewed Apr 19, 2023

View reviewed changes

Merge with master to rebase adding in necessary changes from HDDS-7884.

558347f

Small fix to BasicRootFileSystem.getLinkTarget only suporting bucket …

1aef521

…links. Fixes CI build failure as a result of the recent symlink orphan node patch.

ChenSammi approved these changes Apr 27, 2023

View reviewed changes

ChenSammi merged commit 6a7c070 into apache:master Apr 27, 2023

	return new FileStatusAdapter(0L, 0L, path, true, (short)0, 0L,
	ozoneBucket.getCreationTime().getEpochSecond() * 1000, 0L,
	FsPermission.getDirDefault().toShort(),
	owner, group, null, new BlockLocation[0],

	// ozone delete rm command registration superscedes fs delete
	// ozone delete rm command registration. supercedes fs delete

	public final class OzoneDelete {
	public final class OzoneFsDelete {

	objectStore.getVolume(destVolume).getBucket(destVolume);
	objectStore.getVolume(destVolume).getBucket(srcBucket);

HDDS-7828. Make Ozone fs rm symbolic links command support posix behaviour #4246

HDDS-7828. Make Ozone fs rm symbolic links command support posix behaviour #4246

Uh oh!

Conversation

neils-dev commented Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

neils-dev commented Feb 7, 2023

Uh oh!

Uh oh!

smengcl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neils-dev commented Apr 18, 2023

Uh oh!

ChenSammi commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neils-dev Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Apr 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

neils-dev commented Feb 4, 2023 •

edited

Loading

ChenSammi commented Apr 18, 2023 •

edited

Loading

ChenSammi commented Apr 19, 2023 •

edited

Loading

neils-dev Apr 21, 2023 •

edited

Loading