This is an automated email from the ASF dual-hosted git repository. vjasani pushed a commit to branch branch-3 in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/branch-3 by this push: new 91fdd020d49 HBASE-28042 Snapshot corruptions due to non-atomic rename within same filesystem (#5369) 91fdd020d49 is described below commit 91fdd020d490cc96cbb7b2713bec3096c0e0b1ac Author: Viraj Jasani <vjas...@apache.org> AuthorDate: Sun Aug 27 19:27:05 2023 -0700 HBASE-28042 Snapshot corruptions due to non-atomic rename within same filesystem (#5369) Co-authored-by: Ujjawal <ujjawal4...@gmail.com> Signed-off-by: Wellington Chevreuil <wchevre...@apache.org> Signed-off-by: Abhey Rana <a.r...@salesforce.com> Signed-off-by: Ujjawal <ujjawal4...@gmail.com> Signed-off-by: Aman Poonia <aman.poonia...@gmail.com> --- .../hbase/snapshot/SnapshotDescriptionUtils.java | 37 ++++++++++++++-- .../TestSnapshotScannerHDFSAclController.java | 13 +++--- .../snapshot/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 1e2bbb68c41..689cd89259b 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 @@ -419,11 +419,9 @@ public final class SnapshotDescriptionUtils { // 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) ) { @@ -432,6 +430,37 @@ public final class SnapshotDescriptionUtils { } } + 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 d79e3f30810..99d5a89ac2c 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 class TestSnapshotScannerHDFSAclController { 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 class TestSnapshotScannerHDFSAclController { // 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 class TestSnapshotScannerHDFSAclController { // 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 class TestSnapshotScannerHDFSAclController { // 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 2093baf3607..8e62ef16bbf 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.assertTrue; 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 class TestSnapshotDescriptionUtils { 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)); + } + }