Skip to content
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

HBASE-28042 Snapshot corruptions due to non-atomic rename within same filesystem #5369

Merged
merged 5 commits into from
Aug 28, 2023
Merged

HBASE-28042 Snapshot corruptions due to non-atomic rename within same filesystem #5369

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2898bc3
HBASE-28042 Snapshot corruptions due to non-atomic rename within same…
virajjasani Aug 23, 2023
8ca3b0b
test addendum
virajjasani Aug 24, 2023
880ffd7
addendum
virajjasani Aug 24, 2023
0603f20
addendum - test order changes for read/write perm grant
virajjasani Aug 24, 2023
ce1b14e
addendum
virajjasani Aug 24, 2023
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
40 changes: 36 additions & 4 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,11 +419,9 @@ public static void completeSnapshot(Path snapshotDir, Path workingDir, FileSyste
// if this fails
URI workingURI = workingDirFs.getUri();
URI rootURI = fs.getUri();

if (
(!workingURI.getScheme().equals(rootURI.getScheme()) || workingURI.getAuthority() == null
|| !workingURI.getAuthority().equals(rootURI.getAuthority())
|| workingURI.getUserInfo() == null
|| !workingURI.getUserInfo().equals(rootURI.getUserInfo())
(shouldSkipRenameSnapshotDirectories(workingURI, rootURI)
|| !fs.rename(workingDir, snapshotDir))
&& !FileUtil.copy(workingDirFs, workingDir, fs, snapshotDir, true, true, conf)
) {
Expand All @@ -432,6 +430,40 @@ public static void completeSnapshot(Path snapshotDir, Path workingDir, FileSyste
}
}

static boolean shouldSkipRenameSnapshotDirectories(URI workingURI, URI rootURI) {
// check scheme, e.g. file, hdfs
if (
workingURI.getScheme() == null
&& (rootURI.getScheme() != null && !rootURI.getScheme().equalsIgnoreCase("file"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need the extra check for the "file" URI? Wouldn't it be enough to return "true" here if workingURI scheme is null and rootURI is not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with "file", we could have ignored local fs, but you are right that we don't need such explicit check, let me make this change, thanks

) {
return true;
}
if (workingURI.getScheme() != null && !workingURI.getScheme().equals(rootURI.getScheme())) {
return true;
}

// check Authority, e.g. localhost:port
if (workingURI.getAuthority() == null && rootURI.getAuthority() != null) {
return true;
}
if (
workingURI.getAuthority() != null && !workingURI.getAuthority().equals(rootURI.getAuthority())
) {
return true;
}

// check UGI/userInfo
if (workingURI.getUserInfo() == null && rootURI.getUserInfo() != null) {
return true;
}
if (
workingURI.getUserInfo() != null && !workingURI.getUserInfo().equals(rootURI.getUserInfo())
) {
return true;
}
return false;
}

/**
* Check if the user is this table snapshot's owner
* @param snapshot the table snapshot description
Expand Down
3 changes: 1 addition & 2 deletions ...st/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ public void testGrantGlobal1() throws Exception {

TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table);
snapshotAndWait(snapshot1, table);
snapshotAndWait(snapshot2, table);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test really need this change, or is this part of the PR by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it actually does, i am still trying to understand why the order of granting permissions to global user matter in the order. e.g. before table creation, granting global user permission allows user to scan snapshot but after table creation if we grant it, it doesn't work.
perhaps some bug in coproc SnapshotScannerHDFSAclController

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack!

// grant G(R)
SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, READ);
TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot1, 6);
Expand All @@ -174,8 +175,6 @@ public void testGrantGlobal1() throws Exception {
// grant G(R)
SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, READ);
TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot1, 6);
// take a snapshot and ACLs are inherited automatically
snapshotAndWait(snapshot2, table);
TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot2, 6);
assertTrue(hasUserGlobalHdfsAcl(aclTable, grantUserName));
deleteTable(table);
Expand Down
49 changes: 49 additions & 0 deletions ...e-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static org.junit.Assert.fail;

import java.io.IOException;
import java.net.URI;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
Expand Down Expand Up @@ -191,4 +192,52 @@ public void testIsWithinWorkingDir() throws IOException {
assertTrue(SnapshotDescriptionUtils.isWithinDefaultWorkingDir(
new Path("file:" + hbsaeDir + "/.hbase-snapshot/.tmp/snapshot"), conf));
}

@Test
public void testShouldSkipRenameSnapshotDirectories() {
URI workingDirURI = URI.create("/User/test1");
URI rootDirURI = URI.create("hdfs:///User/test2");

// should skip rename if it's not the same scheme;
assertTrue(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

workingDirURI = URI.create("/User/test1");
rootDirURI = URI.create("file:///User/test2");
assertFalse(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

// skip rename when either scheme or authority are the not same
workingDirURI = URI.create("hdfs://localhost:8020/User/test1");
rootDirURI = URI.create("hdfs://otherhost:8020/User/test2");
assertTrue(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

workingDirURI = URI.create("file:///User/test1");
rootDirURI = URI.create("hdfs://localhost:8020/User/test2");
assertTrue(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

workingDirURI = URI.create("hdfs:///User/test1");
rootDirURI = URI.create("hdfs:///User/test2");
assertFalse(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

workingDirURI = URI.create("hdfs://localhost:8020/User/test1");
rootDirURI = URI.create("hdfs://localhost:8020/User/test2");
assertFalse(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

workingDirURI = URI.create("hdfs://user:password@localhost:8020/User/test1");
rootDirURI = URI.create("hdfs://user:password@localhost:8020/User/test2");
assertFalse(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));

// skip rename when user information is not the same
workingDirURI = URI.create("hdfs://user:password@localhost:8020/User/test1");
rootDirURI = URI.create("hdfs://user2:password2@localhost:8020/User/test2");
assertTrue(
SnapshotDescriptionUtils.shouldSkipRenameSnapshotDirectories(workingDirURI, rootDirURI));
}

}