HBASE-28042 Snapshot corruptions due to non-atomic rename within same…

… filesystem (#5372) (#5369) Co-Authored-By: ujjawal4046 <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Abhey Rana <[email protected]> Signed-off-by: Ujjawal <[email protected]> Signed-off-by: Aman Poonia <[email protected]>
apache · Aug 28, 2023 · 61250ad · 61250ad
1 parent 312f5ee
commit 61250ad
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 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
@@ -409,11 +409,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)
     ) {
@@ -422,6 +420,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/...st/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java b/...st/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/...e-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotDescriptionUtils.java b/...e-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));
+  }
+
 }