HBASE-28658 The failsafe snapshot should be deleted after rollback su…

…ccessfully (#6019) Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit 67d7772)
apache · Jun 22, 2024 · 46995b2 · 46995b2
1 parent 265dcb4
commit 46995b2
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 15 deletions.
diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
@@ -2817,6 +2817,13 @@ public void restoreSnapshot(final String snapshotName, final boolean takeFailSaf
           String msg = "Restore snapshot=" + snapshotName + " failed. Rollback to snapshot="
             + failSafeSnapshotSnapshotName + " succeeded.";
           LOG.error(msg, e);
+          try {
+            LOG.info("Deleting restore-failsafe snapshot: {}", failSafeSnapshotSnapshotName);
+            deleteSnapshot(failSafeSnapshotSnapshotName);
+          } catch (IOException deleteSnapshotException) {
+            LOG.error("Unable to remove the failsafe snapshot: {}", failSafeSnapshotSnapshotName,
+              deleteSnapshotException);
+          }
           throw new RestoreSnapshotException(msg, e);
         } catch (IOException ex) {
           String msg = "Failed to restore and rollback to snapshot=" + failSafeSnapshotSnapshotName;

diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
@@ -2148,10 +2148,21 @@ private CompletableFuture<Void> restoreSnapshot(String snapshotName, TableName t
                     if (err3 != null) {
                       future.completeExceptionally(err3);
                     } else {
-                      String msg =
-                        "Restore snapshot=" + snapshotName + " failed. Rollback to snapshot="
-                          + failSafeSnapshotSnapshotName + " succeeded.";
-                      future.completeExceptionally(new RestoreSnapshotException(msg, err2));
+                      // If fail to restore snapshot but rollback successfully, delete the
+                      // restore-failsafe snapshot.
+                      LOG.info(
+                        "Deleting restore-failsafe snapshot: " + failSafeSnapshotSnapshotName);
+                      addListener(deleteSnapshot(failSafeSnapshotSnapshotName), (ret4, err4) -> {
+                        if (err4 != null) {
+                          LOG.error("Unable to remove the failsafe snapshot: {}",
+                            failSafeSnapshotSnapshotName, err4);
+                        }
+                        String msg =
+                          "Restore snapshot=" + snapshotName + " failed, Rollback to snapshot="
+                            + failSafeSnapshotSnapshotName + " succeeded.";
+                        LOG.error(msg);
+                        future.completeExceptionally(new RestoreSnapshotException(msg, err2));
+                      });
                     }
                   });
               } else {

diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/SnapshotWithAclTestBase.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/SnapshotWithAclTestBase.java
@@ -17,13 +17,20 @@
  */
 package org.apache.hadoop.hbase.client;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThrows;
+
 import java.io.IOException;
 import java.util.List;
 import java.util.regex.Pattern;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hbase.Coprocessor;
 import org.apache.hadoop.hbase.HBaseCommonTestingUtility;
 import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.coprocessor.CoprocessorHost;
 import org.apache.hadoop.hbase.master.MasterCoprocessorHost;
@@ -33,6 +40,8 @@
 import org.apache.hadoop.hbase.security.access.Permission;
 import org.apache.hadoop.hbase.security.access.PermissionStorage;
 import org.apache.hadoop.hbase.security.access.SecureTestUtil;
+import org.apache.hadoop.hbase.snapshot.SnapshotDescriptionUtils;
+import org.apache.hadoop.hbase.snapshot.SnapshotManifest;
 import org.apache.hadoop.hbase.util.Bytes;
 import org.junit.AfterClass;
 import org.junit.Assert;
@@ -110,6 +119,8 @@ public static void setupBeforeClass() throws Exception {
     verifyConfiguration(conf);
     // Enable EXEC permission checking
     conf.setBoolean(AccessControlConstants.EXEC_PERMISSION_CHECKS_KEY, true);
+    TEST_UTIL.getConfiguration().set(HConstants.SNAPSHOT_RESTORE_FAILSAFE_NAME,
+      "hbase-failsafe-{snapshot.name}-{restore.timestamp}");
     TEST_UTIL.startMiniCluster();
     TEST_UTIL.waitUntilAllRegionsAssigned(PermissionStorage.ACL_TABLE_NAME);
     MasterCoprocessorHost cpHost =
@@ -163,7 +174,7 @@ private void verifyRows(TableName tableName) throws IOException {
         byte[] value = result.getValue(TEST_FAMILY, TEST_QUALIFIER);
         Assert.assertArrayEquals(value, Bytes.toBytes(rowCount++));
       }
-      Assert.assertEquals(ROW_COUNT, rowCount);
+      assertEquals(ROW_COUNT, rowCount);
     }
   }
 
@@ -172,7 +183,8 @@ private void verifyRows(TableName tableName) throws IOException {
   protected abstract void cloneSnapshot(String snapshotName, TableName tableName,
     boolean restoreAcl) throws Exception;
 
-  protected abstract void restoreSnapshot(String snapshotName, boolean restoreAcl) throws Exception;
+  protected abstract void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot,
+    boolean restoreAcl) throws Exception;
 
   @Test
   public void testRestoreSnapshot() throws Exception {
@@ -215,7 +227,7 @@ public void testRestoreSnapshot() throws Exception {
 
     // restore snapshot with restoreAcl false.
     TEST_UTIL.getAdmin().disableTable(TEST_TABLE);
-    restoreSnapshot(snapshotName1, false);
+    restoreSnapshot(snapshotName1, false, false);
     TEST_UTIL.getAdmin().enableTable(TEST_TABLE);
     verifyAllowed(new AccessReadAction(TEST_TABLE), USER_OWNER, USER_RW);
     verifyDenied(new AccessReadAction(TEST_TABLE), USER_RO, USER_NONE);
@@ -224,12 +236,36 @@ public void testRestoreSnapshot() throws Exception {
 
     // restore snapshot with restoreAcl true.
     TEST_UTIL.getAdmin().disableTable(TEST_TABLE);
-    restoreSnapshot(snapshotName1, true);
+    restoreSnapshot(snapshotName1, false, true);
     TEST_UTIL.getAdmin().enableTable(TEST_TABLE);
     verifyAllowed(new AccessReadAction(TEST_TABLE), USER_OWNER, USER_RO, USER_RW);
     verifyDenied(new AccessReadAction(TEST_TABLE), USER_NONE);
     verifyAllowed(new AccessWriteAction(TEST_TABLE), USER_OWNER, USER_RW);
     verifyDenied(new AccessWriteAction(TEST_TABLE), USER_RO, USER_NONE);
+
+    // Delete data.manifest of the snapshot to simulate an invalid snapshot.
+    Configuration configuration = TEST_UTIL.getConfiguration();
+    Path rootDir = new Path(configuration.get(HConstants.HBASE_DIR));
+    Path snapshotDir = SnapshotDescriptionUtils.getCompletedSnapshotDir(snapshotName1, rootDir);
+    FileSystem fileSystem = FileSystem.get(rootDir.toUri(), configuration);
+    Path maniFestPath = new Path(snapshotDir, SnapshotManifest.DATA_MANIFEST_NAME);
+    fileSystem.delete(maniFestPath, false);
+    assertFalse(fileSystem.exists(maniFestPath));
+    assertEquals(1, TEST_UTIL.getAdmin().listSnapshots(Pattern.compile(snapshotName1)).size());
+    // There is no failsafe snapshot before restoring.
+    int failsafeSnapshotCount = TEST_UTIL.getAdmin()
+      .listSnapshots(Pattern.compile("hbase-failsafe-" + snapshotName1 + ".*")).size();
+    assertEquals(0, failsafeSnapshotCount);
+    TEST_UTIL.getAdmin().disableTable(TEST_TABLE);
+    // We would get Exception when restoring data by this an invalid snapshot.
+    assertThrows(Exception.class, () -> restoreSnapshot(snapshotName1, true, true));
+    TEST_UTIL.getAdmin().enableTable(TEST_TABLE);
+    verifyRows(TEST_TABLE);
+    // Fail to store snapshot but rollback successfully, so there is no failsafe snapshot after
+    // restoring.
+    failsafeSnapshotCount = TEST_UTIL.getAdmin()
+      .listSnapshots(Pattern.compile("hbase-failsafe-" + snapshotName1 + ".*")).size();
+    assertEquals(0, failsafeSnapshotCount);
   }
 
   final class AccessSnapshotAction implements AccessTestAction {
@@ -257,8 +293,8 @@ public void testDeleteSnapshot() throws Exception {
       USER_RO, USER_RW, USER_NONE);
     List<SnapshotDescription> snapshotDescriptions =
       TEST_UTIL.getAdmin().listSnapshots(Pattern.compile(testSnapshotName));
-    Assert.assertEquals(1, snapshotDescriptions.size());
-    Assert.assertEquals(USER_OWNER.getShortName(), snapshotDescriptions.get(0).getOwner());
+    assertEquals(1, snapshotDescriptions.size());
+    assertEquals(USER_OWNER.getShortName(), snapshotDescriptions.get(0).getOwner());
     AccessTestAction deleteSnapshotAction = () -> {
       try (Connection conn = ConnectionFactory.createConnection(TEST_UTIL.getConfiguration());
         Admin admin = conn.getAdmin()) {
@@ -271,6 +307,6 @@ public void testDeleteSnapshot() throws Exception {
 
     List<SnapshotDescription> snapshotsAfterDelete =
       TEST_UTIL.getAdmin().listSnapshots(Pattern.compile(testSnapshotName));
-    Assert.assertEquals(0, snapshotsAfterDelete.size());
+    assertEquals(0, snapshotsAfterDelete.size());
   }
 }
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSnapshotWithAcl.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSnapshotWithAcl.java
@@ -43,7 +43,8 @@ protected void cloneSnapshot(String snapshotName, TableName tableName, boolean r
   }
 
   @Override
-  protected void restoreSnapshot(String snapshotName, boolean restoreAcl) throws Exception {
-    TEST_UTIL.getAdmin().restoreSnapshot(snapshotName, false, restoreAcl);
+  protected void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot,
+    boolean restoreAcl) throws Exception {
+    TEST_UTIL.getAdmin().restoreSnapshot(snapshotName, takeFailSafeSnapshot, restoreAcl);
   }
 }
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSnapshotWithAclAsyncAdmin.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestSnapshotWithAclAsyncAdmin.java
@@ -49,10 +49,11 @@ protected void cloneSnapshot(String snapshotName, TableName tableName, boolean r
   }
 
   @Override
-  protected void restoreSnapshot(String snapshotName, boolean restoreAcl) throws Exception {
+  protected void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot,
+    boolean restoreAcl) throws Exception {
     try (AsyncConnection conn =
       ConnectionFactory.createAsyncConnection(TEST_UTIL.getConfiguration()).get()) {
-      conn.getAdmin().restoreSnapshot(snapshotName, false, restoreAcl).get();
+      conn.getAdmin().restoreSnapshot(snapshotName, takeFailSafeSnapshot, restoreAcl).get();
     }
   }
 }