HBASE-13182 Test NamespaceAuditor/AccessController create/delete table is flaky

hbase-server/src/test/java/org/apache/hadoop/hbase/namespace/TestNamespaceAuditor.java

@@ -53,9 +53,11 @@ import org.apache.hadoop.hbase.client.HBaseAdmin;
 import org.apache.hadoop.hbase.client.HTable;
 import org.apache.hadoop.hbase.client.Mutation;
 import org.apache.hadoop.hbase.client.Table;
+import org.apache.hadoop.hbase.coprocessor.BaseMasterObserver;
 import org.apache.hadoop.hbase.coprocessor.BaseRegionObserver;
 import org.apache.hadoop.hbase.coprocessor.BaseRegionServerObserver;
 import org.apache.hadoop.hbase.coprocessor.CoprocessorHost;
+import org.apache.hadoop.hbase.coprocessor.MasterCoprocessorEnvironment;
 import org.apache.hadoop.hbase.coprocessor.ObserverContext;
 import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment;
 import org.apache.hadoop.hbase.coprocessor.RegionServerCoprocessorEnvironment;
@@ -93,6 +95,8 @@ public class TestNamespaceAuditor {
   public static void before() throws Exception {
     UTIL.getConfiguration().set(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY,
       CustomObserver.class.getName());
+    UTIL.getConfiguration().set(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY,
+      MasterSyncObserver.class.getName());
     Configuration conf = UTIL.getConfiguration();
     conf.setBoolean(QuotaUtil.QUOTA_CONF_KEY, true);
     conf.setClass("hbase.coprocessor.regionserver.classes", CPRegionServerObserver.class,
@@ -113,10 +117,10 @@ public class TestNamespaceAuditor {
   }
 
   @After
-  public void cleanup() throws IOException, KeeperException {
+  public void cleanup() throws Exception, KeeperException {
     for (HTableDescriptor table : ADMIN.listTables()) {
       ADMIN.disableTable(table.getTableName());
-      ADMIN.deleteTable(table.getTableName());
+      deleteTable(table.getTableName());
     }
     for (NamespaceDescriptor ns : ADMIN.listNamespaceDescriptors()) {
       if (ns.getName().startsWith(prefix)) {
@@ -252,13 +256,13 @@ public class TestNamespaceAuditor {
     assertEquals(5, stateInfo.getRegionCountOfTable(tableDescTwo.getTableName()));
     assertEquals(6, stateInfo.getRegionCount());
     ADMIN.disableTable(tableDescOne.getTableName());
-    ADMIN.deleteTable(tableDescOne.getTableName());
+    deleteTable(tableDescOne.getTableName());
     stateInfo = getNamespaceState(nspDesc.getName());
     assertNotNull("Namespace state found to be null.", stateInfo);
     assertEquals(5, stateInfo.getRegionCount());
     assertEquals(1, stateInfo.getTables().size());
     ADMIN.disableTable(tableDescTwo.getTableName());
-    ADMIN.deleteTable(tableDescTwo.getTableName());
+    deleteTable(tableDescTwo.getTableName());
     ADMIN.deleteNamespace(namespace);
     stateInfo = getNamespaceState(namespace);
     assertNull("Namespace state not found to be null.", stateInfo);
@@ -524,7 +528,7 @@ public class TestNamespaceAuditor {
     ADMIN.createTable(tableDescTwo, Bytes.toBytes("1"), Bytes.toBytes("1000"), 3);
     ADMIN.createTable(tableDescThree, Bytes.toBytes("1"), Bytes.toBytes("1000"), 4);
     ADMIN.disableTable(tableThree);
-    ADMIN.deleteTable(tableThree);
+    deleteTable(tableThree);
     // wait for chore to complete
     UTIL.waitFor(1000, new Waiter.Predicate<Exception>() {
       @Override
@@ -561,4 +565,29 @@ public class TestNamespaceAuditor {
         .getMasterQuotaManager().getNamespaceQuotaManager();
   }
 
+  public static class MasterSyncObserver extends BaseMasterObserver {
+    volatile CountDownLatch tableDeletionLatch;
+
+    @Override
+    public void preDeleteTable(ObserverContext<MasterCoprocessorEnvironment> ctx,
+        TableName tableName) throws IOException {
+      tableDeletionLatch = new CountDownLatch(1);
+    }
+
+    @Override
+    public void postDeleteTableHandler(
+        final ObserverContext<MasterCoprocessorEnvironment> ctx, TableName tableName)
+        throws IOException {
+      tableDeletionLatch.countDown();
+    }
+  }
+
+  private void deleteTable(final 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)UTIL.getHBaseCluster().getMaster()
+      .getMasterCoprocessorHost().findCoprocessor(MasterSyncObserver.class.getName());
+    ADMIN.deleteTable(tableName);
+    observer.tableDeletionLatch.await();
+  }
 }

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

@@ -176,11 +176,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
@@ -280,7 +275,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());
@@ -1074,14 +1069,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
@@ -1330,8 +1323,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) {
@@ -1349,14 +1341,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]);
@@ -1449,8 +1439,7 @@ public class TestAccessController extends SecureTestUtil {
     verifyDenied(user, deleteQualifierAction);
 
     // delete table
-    admin.disableTable(tableName);
+    deleteTable(TEST_UTIL, tableName);
-    admin.deleteTable(tableName);
   }
 
   @Test
@@ -1464,15 +1453,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;
 
@@ -1583,7 +1570,7 @@ public class TestAccessController extends SecureTestUtil {
       hasFoundUserPermission(newOwnerperm, perms));
 
     // delete table
-    admin.deleteTable(tableName);
+    deleteTable(TEST_UTIL, tableName);
   }
 
   @Test
@@ -2007,8 +1994,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
@@ -2147,8 +2133,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();
@@ -2575,8 +2560,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";
@@ -2585,8 +2569,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();
@@ -2609,8 +2592,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

@@ -93,13 +93,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 =