Skip to content

HADOOP-17029. Return correct permission and owner for listing on internal directories in ViewFs #2019

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

Merged
umamaheswararao merged 5 commits into from
Jun 5, 2020
Merged

HADOOP-17029. Return correct permission and owner for listing on internal directories in ViewFs #2019

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35cfe4e
HADOOP-17029. Return correct permission and owner for listing on inte…
abhishekdas99 May 13, 2020
0243a70
HADOOP-17029. Change in ViewFs to cover FileContext based fs impl.
abhishekdas99 May 13, 2020
a7a7875
HADOOP-17029. Fix for dangling links
abhishekdas99 May 14, 2020
da1702a
HADOOP-17029. Addressed code review comments.
abhishekdas99 Jun 3, 2020
7d43b29
HADOOP-17029. Used link.getTargetFileSystem().getUri() for path
abhishekdas99 Jun 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions ...ommon-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1200,13 +1200,24 @@ public FileStatus[] listStatus(Path f) throws AccessControlException,
INode<FileSystem> inode = iEntry.getValue();
if (inode.isLink()) {
INodeLink<FileSystem> link = (INodeLink<FileSystem>) inode;

result[i++] = new FileStatus(0, false, 0, 0,
creationTime, creationTime, PERMISSION_555,
ugi.getShortUserName(), ugi.getPrimaryGroupName(),
link.getTargetLink(),
new Path(inode.fullPath).makeQualified(
myUri, null));
try {
FileStatus status = link.getTargetFileSystem()
.getFileStatus(new Path("/"));
Copy link
Contributor

@umamaheswararao umamaheswararao Jun 4, 2020

Choose a reason for hiding this comment

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

Why are we getting status on "/"? In practical scenario it should work. However what if the target uri is a file?
Should we simply use link.getTargetFileSystem().getUri() ?
Could you please check scenario? If this works, I have no other changes.
Thanks for update.

Copy link
Contributor Author

@abhishekdas99 abhishekdas99 Jun 5, 2020

Choose a reason for hiding this comment

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

Changed to link.getTargetFileSystem().getUri(). Used ChRootedFileSystem to obtain the filestatus. ((ChRootedFileSystem)link.getTargetFileSystem()).getMyFs().getFileStatus().

result[i++] = new FileStatus(status.getLen(), false,
status.getReplication(), status.getBlockSize(),
status.getModificationTime(), status.getAccessTime(),
status.getPermission(), status.getOwner(), status.getGroup(),
link.getTargetLink(),
new Path(inode.fullPath).makeQualified(
myUri, null));
} catch (FileNotFoundException ex) {
result[i++] = new FileStatus(0, false, 0, 0,
creationTime, creationTime, PERMISSION_555,
ugi.getShortUserName(), ugi.getPrimaryGroupName(),
link.getTargetLink(),
new Path(inode.fullPath).makeQualified(
myUri, null));
Comment on lines +1216 to +1221
Copy link
Contributor

@chliang71 chliang71 May 19, 2020

Choose a reason for hiding this comment

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

two comments:

  1. seems to be some duplicate code, the "else" branch is pretty much the same, can we refactor here?
  2. Not sure if this is the best way when dealing with FileNotFoundException. If I understand this correctly, it is possible that some mounts does not have this path, so it can hit FileNotFoundException?

If this is the case, I wonder if it makes more sense to just skip this mount, by not adding a FileStatus for mount at all. So that clients do not get confused by an actually non-existing FileStatus, among other existing ones. But one issue here would be that result array is strictly the size of the # of mounts. Creating result as a list, append, and then return as a array may resolve this.

Copy link
Contributor

@umamaheswararao umamaheswararao May 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For # 2: The problem here I think we may not be able reset mount automatically, so until some one checks file exists or do op on target, we will not know whether the file exists or not. This will continue until user updates mount points accordingly.
This can be possible when some one deletes the target directory directly but not updated the mount tables accordingly. Please check one of my comment on behavior of ls in MAC. Also we have other issue: isDir is inconsistent.

Copy link
Contributor Author

@abhishekdas99 abhishekdas99 Jun 3, 2020

Choose a reason for hiding this comment

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

Sorry if I am missing anything. The change with catching the FileNotFoundException is good ? or I need to make a change.

Copy link
Contributor Author

@abhishekdas99 abhishekdas99 Jun 3, 2020

Choose a reason for hiding this comment

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

Sorry if I am missing anything. The change with catching the FileNotFoundException is good ? or I need to change something. @umamaheswararao

Copy link
Contributor

@umamaheswararao umamaheswararao Jun 4, 2020

Choose a reason for hiding this comment

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

IMO, FileNotFoundException is fine. Please see my comment below https://github.com/apache/hadoop/pull/2019/files#r430611396
@chliang71 , What do you say?

}
} else {
result[i++] = new FileStatus(0, true, 0, 0,
creationTime, creationTime, PERMISSION_555,
Expand Down
38 changes: 31 additions & 7 deletions hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,11 +917,23 @@ public FileStatus getFileLinkStatus(final Path f)
if (inode.isLink()) {
INodeLink<AbstractFileSystem> inodelink =
(INodeLink<AbstractFileSystem>) inode;
result = new FileStatus(0, false, 0, 0, creationTime, creationTime,
try {
FileStatus status = inodelink.getTargetFileSystem()
.getFileStatus(new Path("/"));
result = new FileStatus(status.getLen(), false,
status.getReplication(), status.getBlockSize(),
status.getModificationTime(), status.getAccessTime(),
status.getPermission(), status.getOwner(), status.getGroup(),
inodelink.getTargetLink(),
new Path(inode.fullPath).makeQualified(
myUri, null));
} catch (FileNotFoundException ex) {
Copy link
Contributor

@chliang71 chliang71 May 19, 2020

Choose a reason for hiding this comment

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

similar comment regarding FileNotFoundException. I think in general, it's better to match behavior of non-federated client. If a path does not exist, just throw back FileNotFoundException.

Copy link
Contributor

@umamaheswararao umamaheswararao May 26, 2020

Choose a reason for hiding this comment

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

similar comment regarding FileNotFoundException. I think in general, it's better to match behavior of non-federated client. If a path does not exist, just throw back FileNotFoundException.

I just verified symlinks. When target deleted, ls on symlink does not throw FNFE. Instead it is converted to file link. I tested dir->dir link.
It seems this behavior is correct when compared with other fs. I tested on my MAC.
Should this be fixed in federated clusters is necessary ? Could you please validate this?
If we attempt to open that non existent link file, then we can throw out exception. But ls seems to simply pass.

Work % mkdir linkTarget 
Work % ln -s linkTarget linkSrc
Work % ls -l
lrwxr-xr-x   1 umagangumalla  xxxx     10 May 26 11:08 linkSrc -> linkTarget
Work % rm -rf linkTarget 
Work % ls -l            
lrwxr-xr-x   1 umagangumalla  xxxx     10 May 26 11:08 linkSrc -> linkTarget
Work % cd linkSrc
cd: no such file or directory: linkSrc

result = new FileStatus(0, false, 0, 0, creationTime, creationTime,
PERMISSION_555, ugi.getShortUserName(), ugi.getPrimaryGroupName(),
inodelink.getTargetLink(),
new Path(inode.fullPath).makeQualified(
myUri, null));
}
} else {
result = new FileStatus(0, true, 0, 0, creationTime, creationTime,
PERMISSION_555, ugi.getShortUserName(), ugi.getPrimaryGroupName(),
Expand Down Expand Up @@ -976,12 +988,24 @@ public FileStatus[] listStatus(final Path f) throws AccessControlException,
INodeLink<AbstractFileSystem> link =
(INodeLink<AbstractFileSystem>) inode;

result[i++] = new FileStatus(0, false, 0, 0,
creationTime, creationTime,
PERMISSION_555, ugi.getShortUserName(), ugi.getPrimaryGroupName(),
link.getTargetLink(),
new Path(inode.fullPath).makeQualified(
myUri, null));
try {
FileStatus status = link.getTargetFileSystem()
.getFileStatus(new Path("/"));
result[i++] = new FileStatus(status.getLen(), false,
status.getReplication(), status.getBlockSize(),
status.getModificationTime(), status.getAccessTime(),
status.getPermission(), status.getOwner(), status.getGroup(),
link.getTargetLink(),
new Path(inode.fullPath).makeQualified(
myUri, null));
} catch (FileNotFoundException ex) {
Copy link
Contributor

@chliang71 chliang71 May 19, 2020

Choose a reason for hiding this comment

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

same comment about FileNotFoundException here

result[i++] = new FileStatus(0, false, 0, 0,
creationTime, creationTime, PERMISSION_555,
ugi.getShortUserName(), ugi.getPrimaryGroupName(),
link.getTargetLink(),
new Path(inode.fullPath).makeQualified(
myUri, null));
}
} else {
result[i++] = new FileStatus(0, true, 0, 0,
creationTime, creationTime,
Expand Down
118 changes: 92 additions & 26 deletions ...project/hadoop-common/src/test/java/org/apache/hadoop/fs/viewfs/TestViewfsFileStatus.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,13 @@
import org.apache.hadoop.fs.FsConstants;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.contract.ContractTestUtils;
import org.apache.hadoop.fs.permission.FsPermission;
import org.apache.hadoop.io.DataInputBuffer;
import org.apache.hadoop.io.DataOutputBuffer;
import org.apache.hadoop.test.GenericTestUtils;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mockito;

Expand All @@ -48,6 +51,17 @@ public class TestViewfsFileStatus {
private static final File TEST_DIR = GenericTestUtils.getTestDir(
TestViewfsFileStatus.class.getSimpleName());

@Before
public void setUp() {
FileUtil.fullyDelete(TEST_DIR);
assertTrue(TEST_DIR.mkdirs());
}

@After
public void tearDown() throws IOException {
FileUtil.fullyDelete(TEST_DIR);
}

@Test
public void testFileStatusSerialziation()
throws IOException, URISyntaxException {
Expand All @@ -56,38 +70,90 @@ public void testFileStatusSerialziation()
File infile = new File(TEST_DIR, testfilename);
final byte[] content = "dingos".getBytes();

FileOutputStream fos = null;
try {
fos = new FileOutputStream(infile);
try (FileOutputStream fos = new FileOutputStream(infile)) {
fos.write(content);
} finally {
if (fos != null) {
fos.close();
}
}
assertEquals((long)content.length, infile.length());

Configuration conf = new Configuration();
ConfigUtil.addLink(conf, "/foo/bar/baz", TEST_DIR.toURI());
FileSystem vfs = FileSystem.get(FsConstants.VIEWFS_URI, conf);
assertEquals(ViewFileSystem.class, vfs.getClass());
Path path = new Path("/foo/bar/baz", testfilename);
FileStatus stat = vfs.getFileStatus(path);
assertEquals(content.length, stat.getLen());
ContractTestUtils.assertNotErasureCoded(vfs, path);
assertTrue(path + " should have erasure coding unset in " +
"FileStatus#toString(): " + stat,
stat.toString().contains("isErasureCoded=false"));

// check serialization/deserialization
DataOutputBuffer dob = new DataOutputBuffer();
stat.write(dob);
DataInputBuffer dib = new DataInputBuffer();
dib.reset(dob.getData(), 0, dob.getLength());
FileStatus deSer = new FileStatus();
deSer.readFields(dib);
assertEquals(content.length, deSer.getLen());
assertFalse(deSer.isErasureCoded());
try (FileSystem vfs = FileSystem.get(FsConstants.VIEWFS_URI, conf)) {
assertEquals(ViewFileSystem.class, vfs.getClass());
Path path = new Path("/foo/bar/baz", testfilename);
FileStatus stat = vfs.getFileStatus(path);
assertEquals(content.length, stat.getLen());
ContractTestUtils.assertNotErasureCoded(vfs, path);
assertTrue(path + " should have erasure coding unset in " +
"FileStatus#toString(): " + stat,
stat.toString().contains("isErasureCoded=false"));

// check serialization/deserialization
DataOutputBuffer dob = new DataOutputBuffer();
stat.write(dob);
DataInputBuffer dib = new DataInputBuffer();
dib.reset(dob.getData(), 0, dob.getLength());
FileStatus deSer = new FileStatus();
deSer.readFields(dib);
assertEquals(content.length, deSer.getLen());
assertFalse(deSer.isErasureCoded());
}
}

/**
* Tests the ACL returned from getFileStatus for directories and files.
* @throws IOException
*/
@Test
public void testListStatusACL() throws IOException {
String testfilename = "testFileACL";
String childDirectoryName = "testDirectoryACL";
TEST_DIR.mkdirs();
File infile = new File(TEST_DIR, testfilename);
final byte[] content = "dingos".getBytes();

try (FileOutputStream fos = new FileOutputStream(infile)) {
fos.write(content);
}
assertEquals(content.length, infile.length());
File childDir = new File(TEST_DIR, childDirectoryName);
childDir.mkdirs();

Configuration conf = new Configuration();
ConfigUtil.addLink(conf, "/file", infile.toURI());
ConfigUtil.addLink(conf, "/dir", childDir.toURI());

try (FileSystem vfs = FileSystem.get(FsConstants.VIEWFS_URI, conf)) {
assertEquals(ViewFileSystem.class, vfs.getClass());
FileStatus[] statuses = vfs.listStatus(new Path("/"));

FileSystem localFs = FileSystem.getLocal(conf);
FileStatus fileStat = localFs.getFileStatus(new Path(infile.getPath()));
FileStatus dirStat = localFs.getFileStatus(new Path(childDir.getPath()));

for (FileStatus status : statuses) {
if (status.getPath().getName().equals("file")) {
assertEquals(fileStat.getPermission(), status.getPermission());
} else {
assertEquals(dirStat.getPermission(), status.getPermission());
}
}

localFs.setPermission(new Path(infile.getPath()),
FsPermission.valueOf("-rwxr--r--"));
localFs.setPermission(new Path(childDir.getPath()),
FsPermission.valueOf("-r--rwxr--"));

statuses = vfs.listStatus(new Path("/"));
for (FileStatus status : statuses) {
if (status.getPath().getName().equals("file")) {
assertEquals(FsPermission.valueOf("-rwxr--r--"),
status.getPermission());
} else {
assertEquals(FsPermission.valueOf("-r--rwxr--"),
status.getPermission());
}
}
}
}

// Tests that ViewFileSystem.getFileChecksum calls res.targetFileSystem
Expand Down