HBASE-13182 TestAccessController create/delete table is flaky

hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/SecureTestUtil.java

@@ -28,6 +28,7 @@ import java.security.PrivilegedExceptionAction;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Callable;
+import java.util.concurrent.CountDownLatch;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -35,14 +36,21 @@ import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.Coprocessor;
 import org.apache.hadoop.hbase.HBaseTestingUtility;
 import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.HTableDescriptor;
+import org.apache.hadoop.hbase.HRegionInfo;
 import org.apache.hadoop.hbase.MiniHBaseCluster;
 import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.TableNotEnabledException;
 import org.apache.hadoop.hbase.Waiter.Predicate;
 import org.apache.hadoop.hbase.client.Connection;
 import org.apache.hadoop.hbase.client.ConnectionFactory;
 import org.apache.hadoop.hbase.client.RetriesExhaustedWithDetailsException;
+import org.apache.hadoop.hbase.client.Admin;
 import org.apache.hadoop.hbase.client.Table;
+import org.apache.hadoop.hbase.coprocessor.BaseMasterObserver;
 import org.apache.hadoop.hbase.coprocessor.CoprocessorHost;
+import org.apache.hadoop.hbase.coprocessor.ObserverContext;
+import org.apache.hadoop.hbase.coprocessor.MasterCoprocessorEnvironment;
 import org.apache.hadoop.hbase.io.hfile.HFile;
 import org.apache.hadoop.hbase.protobuf.ProtobufUtil;
 import org.apache.hadoop.hbase.protobuf.generated.AccessControlProtos.AccessControlService;
@@ -68,7 +76,8 @@ public class SecureTestUtil {
   public static void enableSecurity(Configuration conf) throws IOException {
     conf.set("hadoop.security.authorization", "false");
     conf.set("hadoop.security.authentication", "simple");
-    conf.set(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, AccessController.class.getName());
+    conf.set(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, AccessController.class.getName() +
+      "," + MasterSyncObserver.class.getName());
     conf.set(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, AccessController.class.getName() +
       "," + SecureBulkLoadEndpoint.class.getName());
     conf.set(CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, AccessController.class.getName());
@@ -584,4 +593,69 @@ public class SecureTestUtil {
       }
     });
   }
+
+  public static class MasterSyncObserver extends BaseMasterObserver {
+    volatile CountDownLatch tableCreationLatch = null;
+    volatile CountDownLatch tableDeletionLatch = null;
+
+    @Override
+    public void postCreateTableHandler(
+        final ObserverContext<MasterCoprocessorEnvironment> ctx,
+        HTableDescriptor desc, HRegionInfo[] regions) throws IOException {
+      // the AccessController test, some times calls only and directly the postCreateTableHandler()
+      if (tableCreationLatch != null) {
+        tableCreationLatch.countDown();
+      }
+    }
+
+    @Override
+    public void postDeleteTableHandler(
+        final ObserverContext<MasterCoprocessorEnvironment> ctx, TableName tableName)
+        throws IOException {
+      // the AccessController test, some times calls only and directly the postDeleteTableHandler()
+      if (tableDeletionLatch != null) {
+        tableDeletionLatch.countDown();
+      }
+    }
+  }
+
+  public static void createTable(HBaseTestingUtility testUtil, HTableDescriptor htd)
+      throws Exception {
+    createTable(testUtil, testUtil.getHBaseAdmin(), htd);
+  }
+
+  public static void createTable(HBaseTestingUtility testUtil, Admin admin, HTableDescriptor htd)
+      throws Exception {
+    // NOTE: We need a latch because admin is not sync,
+    // so the postOp coprocessor method may be called after the admin operation returned.
+    MasterSyncObserver observer = (MasterSyncObserver)testUtil.getHBaseCluster().getMaster()
+      .getMasterCoprocessorHost().findCoprocessor(MasterSyncObserver.class.getName());
+    observer.tableCreationLatch = new CountDownLatch(1);
+    admin.createTable(htd);
+    observer.tableCreationLatch.await();
+    observer.tableCreationLatch = null;
+    testUtil.waitUntilAllRegionsAssigned(htd.getTableName());
+  }
+
+  public static void deleteTable(HBaseTestingUtility testUtil, TableName tableName)
+      throws Exception {
+    deleteTable(testUtil, testUtil.getHBaseAdmin(), tableName);
+  }
+
+  public static void deleteTable(HBaseTestingUtility testUtil, Admin admin, TableName tableName)
+      throws Exception {
+    // NOTE: We need a latch because admin is not sync,
+    // so the postOp coprocessor method may be called after the admin operation returned.
+    MasterSyncObserver observer = (MasterSyncObserver)testUtil.getHBaseCluster().getMaster()
+      .getMasterCoprocessorHost().findCoprocessor(MasterSyncObserver.class.getName());
+    observer.tableDeletionLatch = new CountDownLatch(1);
+    try {
+      admin.disableTable(tableName);
+    } catch (TableNotEnabledException e) {
+      LOG.debug("Table: " + tableName + " already disabled, so just deleting it.");
+    }
+    admin.deleteTable(tableName);
+    observer.tableDeletionLatch.await();
+    observer.tableDeletionLatch = null;
+  }
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController.java

@@ -173,11 +173,6 @@ public class TestAccessController extends SecureTestUtil {
   public static void setupBeforeClass() throws Exception {
     // setup configuration
     conf = TEST_UTIL.getConfiguration();
-    conf.set("hbase.master.hfilecleaner.plugins",
-      "org.apache.hadoop.hbase.master.cleaner.HFileLinkCleaner," +
-      "org.apache.hadoop.hbase.master.snapshot.SnapshotHFileCleaner");
-    conf.set("hbase.master.logcleaner.plugins",
-      "org.apache.hadoop.hbase.master.snapshot.SnapshotLogCleaner");
     // Enable security
     enableSecurity(conf);
     // In this particular test case, we can't use SecureBulkLoadEndpoint because its doAs will fail
@@ -277,7 +272,7 @@ public class TestAccessController extends SecureTestUtil {
   public void tearDown() throws Exception {
     // Clean the _acl_ table
     try {
-      TEST_UTIL.deleteTable(TEST_TABLE.getTableName());
+      deleteTable(TEST_UTIL, TEST_TABLE.getTableName());
     } catch (TableNotFoundException ex) {
       // Test deleted the table, no problem
       LOG.info("Test deleted table " + TEST_TABLE.getTableName());
@@ -1063,14 +1058,12 @@ public class TestAccessController extends SecureTestUtil {
     // create table
     Admin admin = TEST_UTIL.getHBaseAdmin();
     if (admin.tableExists(tableName)) {
-      admin.disableTable(tableName);
+      deleteTable(TEST_UTIL, tableName);
-      admin.deleteTable(tableName);
     }
     HTableDescriptor htd = new HTableDescriptor(tableName);
     htd.addFamily(new HColumnDescriptor(family1));
     htd.addFamily(new HColumnDescriptor(family2));
-    admin.createTable(htd);
+    createTable(TEST_UTIL, htd);
-    TEST_UTIL.waitUntilAllRegionsAssigned(tableName);
 
     // create temp users
     User tblUser = User
@@ -1319,8 +1312,7 @@ public class TestAccessController extends SecureTestUtil {
     verifyDenied(gblUser, deleteActionAll, deleteAction1, deleteAction2);
 
     // delete table
-    admin.disableTable(tableName);
+    deleteTable(TEST_UTIL, tableName);
-    admin.deleteTable(tableName);
   }
 
   private boolean hasFoundUserPermission(UserPermission userPermission, List<UserPermission> perms) {
@@ -1338,14 +1330,12 @@ public class TestAccessController extends SecureTestUtil {
     // create table
     Admin admin = TEST_UTIL.getHBaseAdmin();
     if (admin.tableExists(tableName)) {
-      admin.disableTable(tableName);
+      deleteTable(TEST_UTIL, tableName);
-      admin.deleteTable(tableName);
     }
     HTableDescriptor htd = new HTableDescriptor(tableName);
     htd.addFamily(new HColumnDescriptor(family1));
     htd.addFamily(new HColumnDescriptor(family2));
-    admin.createTable(htd);
+    createTable(TEST_UTIL, htd);
-    TEST_UTIL.waitUntilAllRegionsAssigned(tableName);
 
     // create temp users
     User user = User.createUserForTesting(TEST_UTIL.getConfiguration(), "user", new String[0]);
@@ -1438,8 +1428,7 @@ public class TestAccessController extends SecureTestUtil {
     verifyDenied(user, deleteQualifierAction);
 
     // delete table
-    admin.disableTable(tableName);
+    deleteTable(TEST_UTIL, tableName);
-    admin.deleteTable(tableName);
   }
 
   @Test
@@ -1453,15 +1442,13 @@ public class TestAccessController extends SecureTestUtil {
     // create table
     Admin admin = TEST_UTIL.getHBaseAdmin();
     if (admin.tableExists(tableName)) {
-      admin.disableTable(tableName);
+      deleteTable(TEST_UTIL, tableName);
-      admin.deleteTable(tableName);
     }
     HTableDescriptor htd = new HTableDescriptor(tableName);
     htd.addFamily(new HColumnDescriptor(family1));
     htd.addFamily(new HColumnDescriptor(family2));
     htd.setOwner(USER_OWNER);
-    admin.createTable(htd);
+    createTable(TEST_UTIL, htd);
-    TEST_UTIL.waitUntilAllRegionsAssigned(tableName);
 
     List<UserPermission> perms;
 
@@ -1572,7 +1559,7 @@ public class TestAccessController extends SecureTestUtil {
       hasFoundUserPermission(newOwnerperm, perms));
 
     // delete table
-    admin.deleteTable(tableName);
+    deleteTable(TEST_UTIL, tableName);
   }
 
   @Test
@@ -1934,8 +1921,7 @@ public class TestAccessController extends SecureTestUtil {
     final Admin admin = TEST_UTIL.getHBaseAdmin();
     HTableDescriptor htd = new HTableDescriptor(TEST_TABLE2);
     htd.addFamily(new HColumnDescriptor(TEST_FAMILY));
-    admin.createTable(htd);
+    createTable(TEST_UTIL, htd);
-    TEST_UTIL.waitUntilAllRegionsAssigned(TEST_TABLE2);
 
     // Starting a new RegionServer.
     JVMClusterUtil.RegionServerThread newRsThread = hbaseCluster
@@ -2074,8 +2060,7 @@ public class TestAccessController extends SecureTestUtil {
         Connection unmanagedConnection = ConnectionFactory.createConnection(TEST_UTIL.getConfiguration());
         Admin admin = unmanagedConnection.getAdmin();
         try {
-          admin.disableTable(TEST_TABLE.getTableName());
+          deleteTable(TEST_UTIL, admin, TEST_TABLE.getTableName());
-          admin.deleteTable(TEST_TABLE.getTableName());
         } finally {
           admin.close();
           unmanagedConnection.close();
@@ -2439,8 +2424,7 @@ public class TestAccessController extends SecureTestUtil {
     Admin admin = TEST_UTIL.getHBaseAdmin();
     HTableDescriptor htd = new HTableDescriptor(table1);
     htd.addFamily(new HColumnDescriptor(family));
-    admin.createTable(htd);
+    createTable(TEST_UTIL, htd);
-    TEST_UTIL.waitUntilAllRegionsAssigned(table1);
 
     // creating the ns and table in it
     String ns = "testNamespace";
@@ -2449,8 +2433,7 @@ public class TestAccessController extends SecureTestUtil {
     TEST_UTIL.getMiniHBaseCluster().getMaster().createNamespace(desc);
     htd = new HTableDescriptor(table2);
     htd.addFamily(new HColumnDescriptor(family));
-    admin.createTable(htd);
+    createTable(TEST_UTIL, htd);
-    TEST_UTIL.waitUntilAllRegionsAssigned(table2);
 
     // Verify that we can read sys-tables
     String aclTableName = AccessControlLists.ACL_TABLE_NAME.getNameAsString();
@@ -2473,8 +2456,8 @@ public class TestAccessController extends SecureTestUtil {
         ns + TableName.NAMESPACE_DELIM + tableName)).size());
     assertEquals(0, testRegexHandler.runAs(getPrivilegedAction("notMatchingAny")).size());
 
-    TEST_UTIL.deleteTable(table1);
+    deleteTable(TEST_UTIL, table1);
-    TEST_UTIL.deleteTable(table2);
+    deleteTable(TEST_UTIL, table2);
     TEST_UTIL.getMiniHBaseCluster().getMaster().deleteNamespace(ns);
   }
 

hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestAccessController2.java

@@ -91,13 +91,13 @@ public class TestAccessController2 extends SecureTestUtil {
         try (Connection connection =
             ConnectionFactory.createConnection(TEST_UTIL.getConfiguration(), testUser)) {
           try (Admin admin = connection.getAdmin()) {
-            admin.createTable(desc);
+            createTable(TEST_UTIL, admin, desc);
           }
         }
         return null;
       }
     }, testUser);
-    TEST_UTIL.waitTableEnabled(TEST_TABLE.getTableName());
+    TEST_UTIL.waitTableAvailable(TEST_TABLE.getTableName());
     // Verify that owner permissions have been granted to the test user on the
     // table just created
     List<TablePermission> perms =