Skip to content

HDFS-14925. Rename operation should check nest snapshot #1670

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
jojochuang merged 4 commits into from Nov 1, 2019
Merged

HDFS-14925. Rename operation should check nest snapshot #1670

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe2d761
Rename operation should check nest snapshot
zhjwpku Oct 23, 2019
c79769a
polish exception message and add unit test
zhjwpku Oct 26, 2019
b6afdb1
fix checkstyle
zhjwpku Oct 27, 2019
20d6afe
add fail() to testRenameWithNestedSnapshottableDirs
zhjwpku Oct 29, 2019
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
51 changes: 39 additions & 12 deletions ...oject/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,8 @@ private static INodesInPath dstForRenameTo(
* Change a path name
*
* @param fsd FSDirectory
* @param src source path
* @param dst destination path
* @param srcIIP source path
* @param dstIIP destination path
* @return true INodesInPath if rename succeeds; null otherwise
* @deprecated See {@link #renameToInt(FSDirectory, String, String,
* boolean, Options.Rename...)}
Expand All @@ -155,8 +155,9 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd,
throws IOException {
assert fsd.hasWriteLock();
final INode srcInode = srcIIP.getLastINode();
List<INodeDirectory> snapshottableDirs = new ArrayList<>();
try {
validateRenameSource(fsd, srcIIP);
validateRenameSource(fsd, srcIIP, snapshottableDirs);
} catch (SnapshotException e) {
throw e;
} catch (IOException ignored) {
Expand Down Expand Up @@ -190,6 +191,8 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd,
return null;
}

validateNestSnapshot(fsd, dstParent.asDirectory(), snapshottableDirs);

fsd.ezManager.checkMoveValidity(srcIIP, dstIIP);
// Ensure dst has quota to accommodate rename
verifyFsLimitsForRename(fsd, srcIIP, dstIIP);
Expand Down Expand Up @@ -342,8 +345,8 @@ static void renameForEditLog(
* for details related to rename semantics and exceptions.
*
* @param fsd FSDirectory
* @param src source path
* @param dst destination path
* @param srcIIP source path
* @param dstIIP destination path
* @param timestamp modification time
* @param collectedBlocks blocks to be removed
* @param options Rename options
Expand All @@ -361,7 +364,8 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,
final String dst = dstIIP.getPath();
final String error;
final INode srcInode = srcIIP.getLastINode();
validateRenameSource(fsd, srcIIP);
List<INodeDirectory> srcSnapshottableDirs = new ArrayList<>();
validateRenameSource(fsd, srcIIP, srcSnapshottableDirs);

// validate the destination
if (dst.equals(src)) {
Expand All @@ -380,10 +384,10 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,
BlockStoragePolicySuite bsps = fsd.getBlockStoragePolicySuite();
fsd.ezManager.checkMoveValidity(srcIIP, dstIIP);
final INode dstInode = dstIIP.getLastINode();
List<INodeDirectory> snapshottableDirs = new ArrayList<>();
List<INodeDirectory> dstSnapshottableDirs = new ArrayList<>();
if (dstInode != null) { // Destination exists
validateOverwrite(src, dst, overwrite, srcInode, dstInode);
FSDirSnapshotOp.checkSnapshot(fsd, dstIIP, snapshottableDirs);
FSDirSnapshotOp.checkSnapshot(fsd, dstIIP, dstSnapshottableDirs);
}

INode dstParent = dstIIP.getINode(-2);
Expand All @@ -400,6 +404,8 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,
throw new ParentNotDirectoryException(error);
}

validateNestSnapshot(fsd, dstParent.asDirectory(), srcSnapshottableDirs);

// Ensure dst has quota to accommodate rename
verifyFsLimitsForRename(fsd, srcIIP, dstIIP);
verifyQuotaForRename(fsd, srcIIP, dstIIP);
Expand Down Expand Up @@ -439,10 +445,10 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,
}
}

if (snapshottableDirs.size() > 0) {
if (dstSnapshottableDirs.size() > 0) {
// There are snapshottable directories (without snapshots) to be
// deleted. Need to update the SnapshotManager.
fsd.getFSNamesystem().removeSnapshottableDirs(snapshottableDirs);
fsd.getFSNamesystem().removeSnapshottableDirs(dstSnapshottableDirs);
}

tx.updateQuotasInSourceTree(bsps);
Expand Down Expand Up @@ -556,7 +562,7 @@ private static void validateOverwrite(
}

private static void validateRenameSource(FSDirectory fsd,
INodesInPath srcIIP) throws IOException {
INodesInPath srcIIP, List<INodeDirectory> snapshottableDirs) throws IOException {
String error;
final INode srcInode = srcIIP.getLastINode();
// validate source
Expand All @@ -574,7 +580,28 @@ private static void validateRenameSource(FSDirectory fsd,
}
// srcInode and its subtree cannot contain snapshottable directories with
// snapshots
FSDirSnapshotOp.checkSnapshot(fsd, srcIIP, null);
FSDirSnapshotOp.checkSnapshot(fsd, srcIIP, snapshottableDirs);
}

private static void validateNestSnapshot(FSDirectory fsd, INodeDirectory dstParent,
List<INodeDirectory> snapshottableDirs) throws SnapshotException {

if (fsd.getFSNamesystem().getSnapshotManager().isAllowNestedSnapshots()) {
return;
}

/*
* snapshottableDirs is a list of snapshottable directory(child of rename src)
* which do not have snapshots yet. If this list is not empty, that means rename
* src have snapshottable descendant directories.
*/
if (snapshottableDirs != null && snapshottableDirs.size() > 0) {
Copy link
Contributor

@jojochuang jojochuang Oct 29, 2019

Choose a reason for hiding this comment

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

I was concerned about the case where the source already has a snapshot.
But turns out that case is already handled before this patch with the following exception
The directory /dir1/foo cannot be deleted since /dir1/foo is snapshottable and already has snapshots at org.apache.hadoop.hdfs.server.namenode.FSDirSnapshotOp.checkSnapshot(FSDirSnapshotOp.java:297)

Copy link
Contributor Author

@zhjwpku zhjwpku Oct 29, 2019

Choose a reason for hiding this comment

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

Yes, that's it ;)

if (fsd.getFSNamesystem().getSnapshotManager().isDescendantOfSnapshotRoot(dstParent)) {
String fullPath = dstParent.getFullPathName();
throw new SnapshotException("Failed to rename to " + fullPath +
" due to nested snapshottable directory");
}
}
}

private static class RenameOperation {
Expand Down
17 changes: 17 additions & 0 deletions ...p-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ void setAllowNestedSnapshots(boolean allowNestedSnapshots) {
this.allowNestedSnapshots = allowNestedSnapshots;
}

public boolean isAllowNestedSnapshots() {
return allowNestedSnapshots;
}

private void checkNestedSnapshottable(INodeDirectory dir, String path)
throws SnapshotException {
if (allowNestedSnapshots) {
Expand Down Expand Up @@ -288,6 +292,19 @@ public INodeDirectory getSnapshottableAncestorDir(final INodesInPath iip)
}
}

public boolean isDescendantOfSnapshotRoot(INodeDirectory dir) {
if (dir.isSnapshottable()) {
return true;
} else {
for (INodeDirectory p = dir; p != null; p = p.getParent()) {
if (this.snapshottables.containsValue(p)) {
return true;
}
}
return false;
}
}

/**
* Create a snapshot of the given path.
* It is assumed that the caller will perform synchronization.
Expand Down