HDFS-14856. Fetch file ACLs while mounting external store. #1478
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
goiri
requested changes
Sep 19, 2019
| @@ -379,6 +379,8 @@ | |||
| public static final String DFS_PROVIDED_ALIASMAP_TEXT_WRITE_DIR_DEFAULT = "file:///tmp/"; | |||
|
|
|||
| public static final String DFS_PROVIDED_ALIASMAP_LEVELDB_PATH = "dfs.provided.aliasmap.leveldb.path"; | |||
| public static final String DFS_NAMENODE_MOUNT_ACLS_ENABLED = "dfs.namenode.mount.acls.enabled"; | |||
There was a problem hiding this comment.
Should we refer to PROVIDED here?
There was a problem hiding this comment.
Agree. WDYT about dfs.namenode.provided.acls.enabled?
Keep namenode since the config is a NN config.
Replaced mount with provided so that the config name is consistent.
There was a problem hiding this comment.
Better than before for sure.
Not sure about the end policy for distinguishing namenode from not.
It should be consistent.
| @@ -55,7 +79,7 @@ public FSTreeWalk(Path root, Configuration conf) throws IOException { | |||
| try { | |||
| ArrayList<TreePath> ret = new ArrayList<>(); | |||
| for (FileStatus s : fs.listStatus(path.getFileStatus().getPath())) { | |||
| ret.add(new TreePath(s, id, i, fs)); | |||
| ret.add(new TreePath(s, id, i, fs, getAclStatus(fs, s.getPath()))); | |||
| getPendingQueue().addFirst( | ||
| new TreePath(p.getFileStatus(), p.getParentId(), this, fs)); | ||
| new TreePath(p.getFileStatus(), p.getParentId(), this, fs, acls)); |
There was a problem hiding this comment.
Do we support adding null to the acls?
Is there a finer grain exception then IOException?
There was a problem hiding this comment.
ACL can be null.
IOException comes from the FileSystems API, AclStatus getAclStatus(Path path) throws IOException
| } | ||
|
|
||
| FSTreeIterator(Path p) throws IOException { | ||
| try { | ||
| FileStatus s = fs.getFileStatus(root); | ||
| getPendingQueue().addFirst(new TreePath(s, -1L, this, fs)); | ||
| AclStatus acls = getAclStatus(fs, s.getPath()); |
There was a problem hiding this comment.
This one we don't care about the IOE?
There was a problem hiding this comment.
Good catch. The scanner will not fail if the remote FS does not support ACLs. All other IOException fail the operation in all paths now. Thanks !
There was a problem hiding this comment.
@ashvina - is it possible to handle the exception the same way in both the constructors?
| @@ -105,5 +150,4 @@ public TreeIterator iterator() { | |||
| throw new RuntimeException(e); | |||
| } | |||
| } | |||
|
|
|||
| .setPermission(permissions); | ||
| if (aclStatus != null) { | ||
| throw new UnsupportedOperationException( | ||
| "Acls not supported by ImageWriter"); |
| @@ -17,6 +17,8 @@ | |||
| */ | |||
| package org.apache.hadoop.hdfs.server.namenode; | |||
|
|
|||
| import org.apache.hadoop.fs.Path; | |||
| * @param aclStatus AclStatus on external store. | ||
| * @return locally mapped user name. | ||
| */ | ||
| public String user(AclStatus aclStatus) { |
There was a problem hiding this comment.
Why do we have this just for doing a get?
There was a problem hiding this comment.
The user mapping behavior can be different in a subclass. For e.g. In this case SingleUGIResolver maps all users to a single user.
| * @param aclStatus remote ACL status. | ||
| * @return local HDFS ACL entries. | ||
| */ | ||
| public List<AclEntry> aclEntries(AclStatus aclStatus) { |
| addGroup(remoteAcl.getGroup()); | ||
| for (AclEntry entry : remoteAcl.getEntries()) { | ||
| // add the users and groups in this acl entry to ugi | ||
| if (entry.getName() != null) { |
|
🎊 +1 overall
This message was automatically generated. |
virajith
reviewed
Sep 20, 2019
| .setPermission(ugi.resolve(s)); | ||
| .setPermission(permissions); | ||
| if (aclStatus != null) { | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
So, this is not currently written to the image?
There was a problem hiding this comment.
Yes. I am proposing to fix the scanner in the PR which is independent of the ImageWriter. The consumer of the scanner can choose to ignore the ACL.
There was a problem hiding this comment.
Can you make this explicit (e.g,, add javadoc for this method)?
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajith
requested changes
Sep 21, 2019
| } | ||
|
|
||
| FSTreeIterator(Path p) throws IOException { | ||
| try { | ||
| FileStatus s = fs.getFileStatus(root); | ||
| getPendingQueue().addFirst(new TreePath(s, -1L, this, fs)); | ||
| AclStatus acls = getAclStatus(fs, s.getPath()); |
There was a problem hiding this comment.
@ashvina - is it possible to handle the exception the same way in both the constructors?
...op-tools/hadoop-fs2img/src/main/java/org/apache/hadoop/hdfs/server/namenode/UGIResolver.java
Show resolved
Hide resolved
| * Validate FSTreeWalk specific behavior | ||
| */ | ||
| public class TestFSTreeWalk { | ||
| // verify that the ACLs are fetched when configured |
There was a problem hiding this comment.
change to javadoc style comment.
goiri
reviewed
Sep 23, 2019
| try { | ||
| return fs.getAclStatus(path); | ||
| } catch (UnsupportedOperationException e) { | ||
| LOG.warn("Remote filesystem {} doesn't support ACLs", fs); |
There was a problem hiding this comment.
Can we have a unit test checking for this string?
goiri
reviewed
Sep 23, 2019
| .setPermission(ugi.resolve(s)); | ||
| .setPermission(permissions); | ||
| if (aclStatus != null) { | ||
| throw new UnsupportedOperationException("ACLs not supported by ImageWriter"); |
goiri
reviewed
Sep 23, 2019
| @@ -132,4 +135,57 @@ public FsPermission permission(FileStatus s) { | |||
| return s.getPermission(); | |||
| } | |||
|
|
|||
| private long resolve(AclStatus aclStatus) { | |||
There was a problem hiding this comment.
No. Static will not work as resolution methods in the subclass may not be static.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajith
approved these changes
Sep 24, 2019
|
💔 -1 overall
This message was automatically generated. |
ashvina
force-pushed
the
fetchRemoteAcls
branch
from
d2434b7 to
73140e1
Compare
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Oct 2, 2019
...tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java
Show resolved
Hide resolved
goiri
reviewed
Oct 2, 2019
...tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java
Outdated
Show resolved
Hide resolved
goiri
approved these changes
Oct 3, 2019
virajith
requested changes
Oct 10, 2019
...tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java
Show resolved
Hide resolved
...tools/hadoop-fs2img/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSTreeWalk.java
Show resolved
Hide resolved
goiri
changed the title
virajith
approved these changes
Oct 11, 2019
ashvina
force-pushed
the
fetchRemoteAcls
branch
from
d2c8ba4 to
1bd5ee0
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Test failures are unrelated. Completing this PR. |
amahussein
pushed a commit
to amahussein/hadoop
that referenced
this pull request
Oct 29, 2019
RogPodge
pushed a commit
to RogPodge/hadoop
that referenced
this pull request
Mar 25, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses https://issues.apache.org/jira/browse/HDFS-14856.
If configuration
dfs.namenode.mount.acls.enabledis set (true),FsTreeWalkwill fetch ACLs of files on the external storage system provided at the time of mount.