SnapshotProcedure and EnableTableProcedure can cause a deadlock

Author: Hernan Gelaf-Romer

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

@@ -42,6 +42,7 @@
 import org.apache.hadoop.hbase.monitoring.MonitoredTask;
 import org.apache.hadoop.hbase.monitoring.TaskMonitor;
 import org.apache.hadoop.hbase.procedure2.Procedure;
+import org.apache.hadoop.hbase.procedure2.ProcedureAbortedException;
 import org.apache.hadoop.hbase.procedure2.ProcedureStateSerializer;
 import org.apache.hadoop.hbase.procedure2.ProcedureSuspendedException;
 import org.apache.hadoop.hbase.procedure2.ProcedureUtil;
@@ -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 {
+            setState(ProcedureState.FAILED);
+            throw new ProcedureAbortedException(
+              "Table " + snapshotTable + " is not in a valid state for snapshot: " + tableState);
           }
           return Flow.HAS_MORE_STATE;
         case SNAPSHOT_SNAPSHOT_ONLINE_REGIONS:

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

@@ -20,15 +20,17 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.fail;
-
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Comparator;
 import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
 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;
@@ -39,7 +41,6 @@
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
-
 import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos;
 import org.apache.hadoop.hbase.shaded.protobuf.generated.SnapshotProtos;
@@ -136,4 +137,46 @@ 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 {
+    master.getTableStateManager().setTableState(TABLE_NAME, TableState.State.ENABLING);
+    final AtomicBoolean failed = new AtomicBoolean(false);
+    Thread snapshotThread = new Thread("snapshot") {
+      @Override
+      public void run() {
+        try {
+          TEST_UTIL.getAdmin().snapshot(snapshot);
+        } catch (IOException e) {
+          failed.set(true);
+        }
+      }
+    };
+    snapshotThread.start();
+    SnapshotManager sm = master.getSnapshotManager();
+    TEST_UTIL.waitFor(60000, 50,
+      () -> !sm.isTakingSnapshot(TABLE_NAME) && sm.isTableTakingAnySnapshot(TABLE_NAME));
+
+    TestEnableTableProcedure enableTable =
+      new TestEnableTableProcedure(master.getMasterProcedureExecutor().getEnvironment(), TABLE_NAME,
+        MasterProcedureProtos.EnableTableState.ENABLE_TABLE_SET_ENABLED_TABLE_STATE);
+    master.getMasterProcedureExecutor().submitProcedure(enableTable);
+    TEST_UTIL.waitFor(60000,
+      () -> master.getTableStateManager().getTableState(TABLE_NAME).isEnabled() && failed.get());
+    TEST_UTIL.waitFor(60000, failed::get);
+  }
+
+  // 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);
+    }
+
+  }
 }