SnapshotProcedure and EnableTableProcedure can cause a deadlock

Author: Hernan Gelaf-Romer

hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/SnapshotProcedure.java

@@ -26,6 +26,7 @@
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HBaseIOException;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.client.RegionInfo;
 import org.apache.hadoop.hbase.client.RegionReplicaUtil;
@@ -143,13 +144,18 @@ protected Flow executeFromState(MasterProcedureEnv env, SnapshotState state)
           setNextState(SnapshotState.SNAPSHOT_WRITE_SNAPSHOT_INFO);
           return Flow.HAS_MORE_STATE;
         case SNAPSHOT_WRITE_SNAPSHOT_INFO:
-          SnapshotDescriptionUtils.writeSnapshotInfo(snapshot, workingDir, workingDirFS);
           TableState tableState =
             env.getMasterServices().getTableStateManager().getTableState(snapshotTable);
           if (tableState.isEnabled()) {
+            SnapshotDescriptionUtils.writeSnapshotInfo(snapshot, workingDir, workingDirFS);
             setNextState(SnapshotState.SNAPSHOT_SNAPSHOT_ONLINE_REGIONS);
           } else if (tableState.isDisabled()) {
+            SnapshotDescriptionUtils.writeSnapshotInfo(snapshot, workingDir, workingDirFS);
             setNextState(SnapshotState.SNAPSHOT_SNAPSHOT_CLOSED_REGIONS);
+          } else {
+            setFailureState(new HBaseIOException(
+              "Table " + snapshotTable + " is not in a valid state for snapshot: " + tableState));
+            return Flow.NO_MORE_STATE;
           }
           return Flow.HAS_MORE_STATE;
         case SNAPSHOT_SNAPSHOT_ONLINE_REGIONS:
@@ -195,9 +201,7 @@ protected Flow executeFromState(MasterProcedureEnv env, SnapshotState state)
     } catch (ProcedureSuspendedException e) {
       throw e;
     } catch (Exception e) {
-      setFailure("master-snapshot", e);
-      LOG.warn("unexpected exception while execute {}. Mark procedure Failed.", this, e);
-      status.abort("Abort Snapshot " + snapshot.getName() + " on Table " + snapshotTable);
+      setFailureState(e);
       return Flow.NO_MORE_STATE;
     }
   }
@@ -236,6 +240,12 @@ protected SnapshotState getInitialState() {
     return SnapshotState.SNAPSHOT_PREPARE;
   }
 
+  private void setFailureState(Exception e) {
+    setFailure("master-snapshot", e);
+    LOG.warn("unexpected exception while execute {}. Mark procedure Failed.", this, e);
+    status.abort("Abort Snapshot " + snapshot.getName() + " on Table " + snapshotTable);
+  }
+
   private void prepareSnapshot(MasterProcedureEnv env)
     throws ProcedureSuspendedException, IOException {
     if (isAnySplitOrMergeProcedureRunning(env)) {

hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestSnapshotProcedureConcurrently.java

@@ -27,8 +27,10 @@
 import java.util.List;
 import java.util.stream.Collectors;
 import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.client.SnapshotDescription;
 import org.apache.hadoop.hbase.client.SnapshotType;
+import org.apache.hadoop.hbase.client.TableState;
 import org.apache.hadoop.hbase.master.snapshot.SnapshotManager;
 import org.apache.hadoop.hbase.procedure2.Procedure;
 import org.apache.hadoop.hbase.procedure2.ProcedureExecutor;
@@ -136,4 +138,40 @@ public void run() {
     SnapshotTestingUtils.confirmSnapshotValid(TEST_UTIL, snapshotProto, TABLE_NAME, CF);
     SnapshotTestingUtils.confirmSnapshotValid(TEST_UTIL, snapshotOnSameTableProto, TABLE_NAME, CF);
   }
+
+  @Test
+  public void testItFailsIfTableIsNotDisabledOrEnabled() throws Exception {
+    ProcedureExecutor<MasterProcedureEnv> executor = master.getMasterProcedureExecutor();
+    MasterProcedureEnv env = executor.getEnvironment();
+    master.getTableStateManager().setTableState(TABLE_NAME, TableState.State.ENABLING);
+    // Using a delayed spy ensures we hit the problem state while the table enable procedure
+    // is waiting to run
+    SnapshotProcedure snapshotProc =
+      getDelayedOnSpecificStateSnapshotProcedure(new SnapshotProcedure(env, snapshotProto), env,
+        MasterProcedureProtos.SnapshotState.SNAPSHOT_WRITE_SNAPSHOT_INFO);
+    long snapshotProcId = executor.submitProcedure(snapshotProc);
+    TEST_UTIL.waitFor(2000, () -> master.getProcedures().stream().map(Procedure::getProcId)
+      .collect(Collectors.toSet()).contains(snapshotProcId));
+
+    TestEnableTableProcedure enableTable =
+      new TestEnableTableProcedure(master.getMasterProcedureExecutor().getEnvironment(), TABLE_NAME,
+        MasterProcedureProtos.EnableTableState.ENABLE_TABLE_SET_ENABLED_TABLE_STATE);
+    executor.submitProcedure(enableTable);
+    TEST_UTIL.waitFor(70000, // need to wait slightly longer than the spy proc (60_000)
+      () -> master.getTableStateManager().getTableState(TABLE_NAME).isEnabled());
+  }
+
+  // Needs to be publicly accessible for Procedure validation
+  public static class TestEnableTableProcedure extends EnableTableProcedure {
+    // Necessary for Procedure validation
+    public TestEnableTableProcedure() {
+    }
+
+    public TestEnableTableProcedure(MasterProcedureEnv env, TableName tableName,
+      MasterProcedureProtos.EnableTableState state) {
+      super(env, tableName);
+      this.setNextState(state);
+    }
+
+  }
 }