Skip to content

HDFS-15607. Addendum: Create trash dir when allowing snapshottable dir #2448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 10, 2020
Merged

HDFS-15607. Addendum: Create trash dir when allowing snapshottable dir #2448

merged 1 commit into from
Nov 10, 2020

Conversation

@smengcl
Copy link
Contributor

@smengcl smengcl commented Nov 9, 2020 •
edited
Loading

This is an addendum to HDFS-15607's #2352.

Problem

Pre HDFS-15607, when admin disallows snapshot on a file, it throws PathIsNotDirectoryException:

org.apache.hadoop.fs.PathIsNotDirectoryException: `/ssdir1/file1': Is not a directory
	at org.apache.hadoop.hdfs.server.namenode.INodeDirectory.valueOf(INodeDirectory.java:65)
	at org.apache.hadoop.hdfs.server.namenode.snapshot.SnapshotManager.resetSnapshottable(SnapshotManager.java:289)
	at org.apache.hadoop.hdfs.server.namenode.FSDirSnapshotOp.disallowSnapshot(FSDirSnapshotOp.java:76)
	at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.disallowSnapshot(FSNamesystem.java:6933)
	at org.apache.hadoop.hdfs.server.namenode.NameNodeRpcServer.disallowSnapshot(NameNodeRpcServer.java:1969)
	at org.apache.hadoop.hdfs.protocolPB.ClientNamenodeProtocolServerSideTranslatorPB.disallowSnapshot(ClientNamenodeProtocolServerSideTranslatorPB.java:1321)

After HDFS-15607 (current), the thrown exception changed into AccessControlException. Because DFS#checkTrashRootAndRemoveIfEmpty is calling DFS#listStatus on a file (/ssdir1/file1/.Trash) inside a file (/ssdir1/file1):

2020-11-09 09:50:18,374 [IPC Server handler 3 on default port 52295] INFO  FSNamesystem.audit (FSNamesystem.java:logAuditMessage(8708)) - allowed=false	ugi=smeng (auth:SIMPLE)	ip=/127.0.0.1	cmd=listStatus	src=/ssdir1/file1/.Trash	dst=null	perm=null	proto=rpc
2020-11-09 09:50:18,374 [IPC Server handler 3 on default port 52295] INFO  ipc.Server (Server.java:logException(3006)) - IPC Server handler 3 on default port 52295, call Call#31 Retry#0 org.apache.hadoop.hdfs.protocol.ClientProtocol.getListing from 127.0.0.1:52305: org.apache.hadoop.security.AccessControlException: /ssdir1/file1 (is not a directory)
disallowSnapshot: /ssdir1/file1 (is not a directory)
	at org.apache.hadoop.hdfs.server.namenode.FSDirectory.resolvePath(FSDirectory.java:739)
	at org.apache.hadoop.hdfs.server.namenode.FSDirStatAndListingOp.getListingInt(FSDirStatAndListingOp.java:57)
	at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.getListing(FSNamesystem.java:4132)
	at org.apache.hadoop.hdfs.server.namenode.NameNodeRpcServer.getListing(NameNodeRpcServer.java:1175)
	at org.apache.hadoop.hdfs.protocolPB.ClientNamenodeProtocolServerSideTranslatorPB.getListing(ClientNamenodeProtocolServerSideTranslatorPB.java:758)

Though this is a rare use case, we'd better fix this just in case some snippets/scripts are expecting PathIsNotDirectoryException.

Solution

We should ignore AccessControlException thrown in DFS#checkTrashRootAndRemoveIfEmpty and let the original dfs.disallowSnapshot logic handle it.

Testing

After this change, disallowing snapshot on a file throws PathIsNotDirectoryException as expected:

2020-11-09 09:49:15,347 [IPC Server handler 4 on default port 52270] INFO  ipc.Server (Server.java:logException(3013)) - IPC Server handler 4 on default port 52270, call Call#30 Retry#0 org.apache.hadoop.hdfs.protocol.ClientProtocol.disallowSnapshot from 127.0.0.1:52280
org.apache.hadoop.fs.PathIsNotDirectoryException: `/ssdir1/file1': Is not a directory
	at org.apache.hadoop.hdfs.server.namenode.INodeDirectory.valueOf(INodeDirectory.java:65)
	at org.apache.hadoop.hdfs.server.namenode.snapshot.SnapshotManager.resetSnapshottable(SnapshotManager.java:289)
	at org.apache.hadoop.hdfs.server.namenode.FSDirSnapshotOp.disallowSnapshot(FSDirSnapshotOp.java:76)
	at org.apache.hadoop.hdfs.server.namenode.FSNamesystem.disallowSnapshot(FSNamesystem.java:6933)
	at org.apache.hadoop.hdfs.server.namenode.NameNodeRpcServer.disallowSnapshot(NameNodeRpcServer.java:1969)

@smengcl
HDFS-15607. Addendum: Create trash dir when allowing snapshottable dir
0ade21b 
Change-Id: I29e105fa99fd3729496002f2d0c11de37db45193
@smengcl smengcl added the bug label Nov 9, 2020
@smengcl smengcl requested a review from jojochuang November 9, 2020 18:42
@smengcl smengcl self-assigned this Nov 9, 2020
@hadoop-yetus
Copy link

hadoop-yetus commented Nov 9, 2020

💔 -1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 1m 15s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
-1 ❌ test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ trunk Compile Tests _
+1 💚 mvninstall 36m 19s trunk passed
+1 💚 compile 0m 56s trunk passed with JDK Ubuntu-11.0.9+11-Ubuntu-0ubuntu1.18.04.1
+1 💚 compile 0m 48s trunk passed with JDK Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
+1 💚 checkstyle 0m 24s trunk passed
+1 💚 mvnsite 0m 54s trunk passed
+1 💚 shadedclient 18m 0s branch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 38s trunk passed with JDK Ubuntu-11.0.9+11-Ubuntu-0ubuntu1.18.04.1
+1 💚 javadoc 0m 33s trunk passed with JDK Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
+0 🆗 spotbugs 2m 34s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 2m 31s trunk passed
_ Patch Compile Tests _
+1 💚 mvninstall 0m 49s the patch passed
+1 💚 compile 0m 52s the patch passed with JDK Ubuntu-11.0.9+11-Ubuntu-0ubuntu1.18.04.1
+1 💚 javac 0m 52s the patch passed
+1 💚 compile 0m 45s the patch passed with JDK Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
+1 💚 javac 0m 45s the patch passed
+1 💚 checkstyle 0m 17s the patch passed
+1 💚 mvnsite 0m 46s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 shadedclient 16m 27s patch has no errors when building and testing our client artifacts.
+1 💚 javadoc 0m 33s the patch passed with JDK Ubuntu-11.0.9+11-Ubuntu-0ubuntu1.18.04.1
+1 💚 javadoc 0m 30s the patch passed with JDK Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
+1 💚 findbugs 2m 36s the patch passed
_ Other Tests _
+1 💚 unit 2m 12s hadoop-hdfs-client in the patch passed.
+1 💚 asflicense 0m 29s The patch does not generate ASF License warnings.
91m 7s
Subsystem Report/Notes
Docker ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2448/1/artifact/out/Dockerfile
GITHUB PR #2448
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 463b05846ca9 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/bin/hadoop.sh
git revision trunk / ae7b00a
Default Java Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.9+11-Ubuntu-0ubuntu1.18.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
Test Results https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2448/1/testReport/
Max. process+thread count 310 (vs. ulimit of 5500)
modules C: hadoop-hdfs-project/hadoop-hdfs-client U: hadoop-hdfs-project/hadoop-hdfs-client
Console output https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2448/1/console
versions git=2.17.1 maven=3.6.0 findbugs=4.1.3
Powered by Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

@smengcl smengcl requested a review from bshashikant November 10, 2020 16:46
jojochuang
jojochuang approved these changes Nov 10, 2020
View reviewed changes
Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We change the exception thrown by disallowSnapshot. But this is a privileged operation so very unlikely to impact client applications.

@smengcl
Copy link
Contributor Author

smengcl commented Nov 10, 2020

LGTM. We change the exception thrown by disallowSnapshot. But this is a privileged operation so very unlikely to impact client applications.

Yes indeed. Also this is a rare and have mild consequences. I wouldn't expect admins to encounter this daily. :)

Thanks for the +1. Will commit shortly.

@smengcl smengcl merged commit fbd2220 into apache:trunk Nov 10, 2020
jojochuang pushed a commit to jojochuang/hadoop that referenced this pull request May 23, 2023
@smengcl @arp7
HDFS-15607. Addendum: Create trash dir when allowing snapshottable dir (
26d127c 
apache#2448)

(cherry picked from commit fbd2220)

Conflicts:
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java

Change-Id: I642de9b9a102b3955558c2d330809bc5e053690d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jojochuang jojochuang jojochuang approved these changes

@bshashikant bshashikant Awaiting requested review from bshashikant

Assignees

@smengcl smengcl

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@smengcl @hadoop-yetus @jojochuang