From d906a930e11c1ff14a29c349bc5c9d468834bebd Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Thu, 24 Aug 2023 14:18:33 -0700 Subject: [PATCH 1/2] HBASE-28042 Snapshot corruptions due to non-atomic rename within same filesystem --- .../snapshot/SnapshotDescriptionUtils.java | 37 ++++++++++++-- .../TestSnapshotScannerHDFSAclController.java | 13 +++-- .../TestSnapshotDescriptionUtils.java | 49 +++++++++++++++++++ 3 files changed, 88 insertions(+), 11 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java index 6b61a2eab06a..507c28caaf8f 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/SnapshotDescriptionUtils.java @@ -435,11 +435,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) ) { @@ -448,6 +446,37 @@ 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) { + 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 diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java index 8369f840ea5c..20784e70e43f 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java @@ -158,6 +158,7 @@ public void testGrantGlobal1() throws Exception { TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table); snapshotAndWait(snapshot1, table); + snapshotAndWait(snapshot2, table); // grant G(R) SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, READ); TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot1, 6); @@ -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); @@ -197,10 +196,10 @@ public void testGrantGlobal2() throws Exception { // create table in namespace1 and snapshot TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table1); snapshotAndWait(snapshot1, table1); - admin.grant(new UserPermission(grantUserName, - Permission.newBuilder(namespace1).withActions(READ).build()), false); // grant G(W) SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, WRITE); + admin.grant(new UserPermission(grantUserName, + Permission.newBuilder(namespace1).withActions(READ).build()), false); // create table in namespace2 and snapshot TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table2); snapshotAndWait(snapshot2, table2); @@ -231,11 +230,11 @@ public void testGrantGlobal3() throws Exception { // grant table1(R) TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table1); snapshotAndWait(snapshot1, table1); - TestHDFSAclHelper.grantOnTable(TEST_UTIL, grantUserName, table1, READ); - // grant G(W) - SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, WRITE); TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table2); snapshotAndWait(snapshot2, table2); + // grant G(W) + SecureTestUtil.grantGlobal(TEST_UTIL, grantUserName, WRITE); + TestHDFSAclHelper.grantOnTable(TEST_UTIL, grantUserName, table1, READ); // check scan snapshot TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot1, 6); TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot2, -1); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java index cac6f44ff950..9a7a683857cc 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java @@ -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; @@ -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"); + assertTrue( + 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)); + } + } From 4f2caea89f078f307f713a6d0a289c2d8d844cc7 Mon Sep 17 00:00:00 2001 From: Viraj Jasani Date: Fri, 25 Aug 2023 16:15:48 -0700 Subject: [PATCH 2/2] addendum --- .../hadoop/hbase/io/hfile/bucket/TestVerifyBucketCacheFile.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestVerifyBucketCacheFile.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestVerifyBucketCacheFile.java index f0d612930820..64dcd6c7067c 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestVerifyBucketCacheFile.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestVerifyBucketCacheFile.java @@ -36,8 +36,8 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.HBaseClassTestRule; -import org.apache.hadoop.hbase.HBaseTestingUtility; import org.apache.hadoop.hbase.HBaseConfiguration; +import org.apache.hadoop.hbase.HBaseTestingUtility; import org.apache.hadoop.hbase.io.hfile.BlockCacheKey; import org.apache.hadoop.hbase.io.hfile.CacheConfig; import org.apache.hadoop.hbase.io.hfile.CacheTestUtils;