HBASE-22578 HFileCleaner should not delete empty ns/table directories…

… used for user san snapshot feature
apache · Jun 25, 2019 · fc69c10 · fc69c10
1 parent 15ac781
commit fc69c10
Show file tree

Hide file tree

Showing 4 changed files with 153 additions and 5 deletions.
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/CleanerChore.java
@@ -407,6 +407,15 @@ private boolean checkAndDeleteFiles(List<FileStatus> files) {
     return deleteFiles(filesToDelete) == files.size();
   }
 
+  /**
+   * Check if the directory can be deleted
+   * @param dir The path of a directory
+   * @return True if the directory can be deleted, otherwise false
+   */
+  boolean shouldDeleteDirectory(Path dir) {
+    return true;
+  }
+
   /**
    * Delete the given files
    * @param filesToDelete files to delete
@@ -515,8 +524,8 @@ protected Boolean compute() {
 
       boolean result = allFilesDeleted && allSubdirsDeleted;
       // if and only if files and subdirs under current dir are deleted successfully, and
-      // it is not the root dir, then task will try to delete it.
-      if (result && !root) {
+      // it is not the root dir, and the directory can be deleted, then task will try to delete it.
+      if (result && !root && shouldDeleteDirectory(dir)) {
         result &= deleteAction(() -> fs.delete(dir, false), "dir");
       }
       return result;

diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/HFileCleaner.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/cleaner/HFileCleaner.java
@@ -33,8 +33,8 @@
 import org.apache.hadoop.hbase.Stoppable;
 import org.apache.hadoop.hbase.io.HFileLink;
 import org.apache.hadoop.hbase.master.HMaster;
-import org.apache.hadoop.hbase.master.MasterServices;
 import org.apache.hadoop.hbase.regionserver.StoreFileInfo;
+import org.apache.hadoop.hbase.security.access.SnapshotScannerHDFSAclHelper;
 import org.apache.hadoop.hbase.util.StealJobQueue;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
@@ -46,7 +46,7 @@
  */
 @InterfaceAudience.Private
 public class HFileCleaner extends CleanerChore<BaseHFileCleanerDelegate> {
-  private MasterServices master;
+  private HMaster master;
 
   public static final String MASTER_HFILE_CLEANER_PLUGINS = "hbase.master.hfilecleaner.plugins";
 
@@ -103,6 +103,7 @@ public HFileCleaner(final int period, final Stoppable stopper, Configuration con
   private long cleanerThreadCheckIntervalMsec;
   private List<Thread> threads = new ArrayList<Thread>();
   private boolean running;
+  private boolean userScanSnapshotEnabled;
 
   private AtomicLong deletedLargeFiles = new AtomicLong();
   private AtomicLong deletedSmallFiles = new AtomicLong();
@@ -136,6 +137,10 @@ public HFileCleaner(final int period, final Stoppable stopper, Configuration con
     cleanerThreadCheckIntervalMsec =
         conf.getLong(HFILE_DELETE_THREAD_CHECK_INTERVAL_MSEC,
             DEFAULT_HFILE_DELETE_THREAD_CHECK_INTERVAL_MSEC);
+    userScanSnapshotEnabled = SnapshotScannerHDFSAclHelper.isUserScanSnapshotEnabled(conf);
+    if (userScanSnapshotEnabled) {
+      init(params);
+    }
     startHFileDeleteThreads();
   }
 
@@ -502,7 +507,21 @@ public synchronized void cancel(boolean mayInterruptIfRunning) {
 
   public void init(Map<String, Object> params) {
     if (params != null && params.containsKey(HMaster.MASTER)) {
-      this.master = (MasterServices) params.get(HMaster.MASTER);
+      this.master = (HMaster) params.get(HMaster.MASTER);
     }
   }
+
+  @Override
+  boolean shouldDeleteDirectory(Path dir) {
+    /*
+     * If user scan snapshot feature is enabled(see HBASE-21995), then when namespace or table
+     * exists, the archive namespace or table directories should not be deleted because the HDFS
+     * acls are set at these directories; the archive data directory should not be deleted because
+     * the HDFS acls of global permission is set at this directory.
+     */
+    /*if (userScanSnapshotEnabled) {
+      return SnapshotScannerHDFSAclHelper.shouldDeleteArchiveDirectory(dir, master);
+    }*/
+    return true;
+  }
 }
diff --git a/...r/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java b/...r/src/main/java/org/apache/hadoop/hbase/security/access/SnapshotScannerHDFSAclHelper.java
@@ -47,17 +47,21 @@
 import org.apache.hadoop.fs.permission.FsPermission;
 import org.apache.hadoop.hbase.AuthUtil;
 import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.MetaTableAccessor;
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.client.Admin;
 import org.apache.hadoop.hbase.client.Connection;
 import org.apache.hadoop.hbase.client.SnapshotDescription;
 import org.apache.hadoop.hbase.client.TableDescriptor;
+import org.apache.hadoop.hbase.coprocessor.CoprocessorHost;
+import org.apache.hadoop.hbase.master.HMaster;
 import org.apache.hadoop.hbase.mob.MobUtils;
 import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.yetus.audience.InterfaceAudience;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting;
 import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
 import org.apache.hbase.thirdparty.com.google.common.collect.Sets;
 import org.apache.hbase.thirdparty.com.google.common.util.concurrent.ThreadFactoryBuilder;
@@ -515,6 +519,84 @@ private static AclEntry aclEntry(AclEntryScope scope, String name) {
         .setPermission(READ_EXECUTE).build();
   }
 
+  public static boolean isUserScanSnapshotEnabled(Configuration conf) {
+    String masterCoprocessors = conf.get(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
+    if (conf.getBoolean(USER_SCAN_SNAPSHOT_ENABLE, false)
+        && masterCoprocessors.contains(SnapshotScannerHDFSAclController.class.getName())
+        && masterCoprocessors.contains(AccessController.class.getName())) {
+      return true;
+    }
+    return false;
+  }
+
+  /**
+   * If user scan snapshot feature is enabled, then when namespace or table exists, the archive
+   * namespace or table directories should not be deleted because the HDFS acls are set at these
+   * directories. The archive data directory should not be deleted because the HDFS acls of global
+   * permission is set at this directory.
+   */
+  public static boolean shouldDeleteArchiveDirectory(Path dir, HMaster master) {
+    if (isArchiveDataDir(dir)) {
+      return false;
+    } else if (isArchiveNamespaceDir(dir) && namespaceExists(dir.getName(), master)) {
+      return false;
+    } else if (isArchiveTableDir(dir)) {
+      String name = dir.getName();
+      String namespace = dir.getParent().getName();
+      if (tableExists(TableName.valueOf(namespace, name), master)) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  @VisibleForTesting
+  static boolean isArchiveDataDir(Path path) {
+    if (path != null && path.getName().equals(HConstants.BASE_NAMESPACE_DIR)) {
+      Path parent = path.getParent();
+      return parent != null && parent.getName().equals(HConstants.HFILE_ARCHIVE_DIRECTORY);
+    }
+    return false;
+  }
+
+  @VisibleForTesting
+  static boolean isArchiveNamespaceDir(Path path) {
+    return path == null ? false : isArchiveDataDir(path.getParent());
+  }
+
+  @VisibleForTesting
+  static boolean isArchiveTableDir(Path path) {
+    return path == null ? false : isArchiveNamespaceDir(path.getParent());
+  }
+
+  private static boolean namespaceExists(String namespace, HMaster master) {
+    try {
+      if (master != null) {
+        return master.listNamespaces().contains(namespace);
+      } else {
+        LOG.warn("Master is null when check if namespace {} exists", namespace);
+        return false;
+      }
+    } catch (IOException e) {
+      LOG.warn("Check if namespace {} exists error", namespace, e);
+      return true;
+    }
+  }
+
+  private static boolean tableExists(TableName tableName, HMaster master) {
+    try {
+      if (master != null) {
+        return MetaTableAccessor.tableExists(master.getConnection(), tableName);
+      } else {
+        LOG.warn("Master is null when check if table {} exists", tableName);
+        return false;
+      }
+    } catch (IOException e) {
+      LOG.error("Check if table {} exists error", tableName, e);
+      return true;
+    }
+  }
+
   /**
    * Inner class used to describe modify or remove what type of acl entries(ACCESS, DEFAULT,
    * ACCESS_AND_DEFAULT) for files or directories(and child files).

diff --git a/...st/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java b/...st/java/org/apache/hadoop/hbase/security/access/TestSnapshotScannerHDFSAclController.java
@@ -21,6 +21,8 @@
 import static org.apache.hadoop.hbase.security.access.Permission.Action.READ;
 import static org.apache.hadoop.hbase.security.access.Permission.Action.WRITE;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
 import java.io.IOException;
 import java.security.PrivilegedExceptionAction;
@@ -46,11 +48,14 @@
 import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
 import org.apache.hadoop.hbase.client.TableSnapshotScanner;
 import org.apache.hadoop.hbase.coprocessor.CoprocessorHost;
+import org.apache.hadoop.hbase.master.cleaner.HFileCleaner;
 import org.apache.hadoop.hbase.security.User;
 import org.apache.hadoop.hbase.testclassification.LargeTests;
 import org.apache.hadoop.hbase.testclassification.SecurityTests;
 import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.hbase.util.HFileArchiveUtil;
 import org.junit.AfterClass;
+import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.ClassRule;
 import org.junit.Rule;
@@ -546,6 +551,39 @@ public void testDeleteNamespace() throws Exception {
     }
   }
 
+  @Test
+  public void testCleanArchiveTableDir() throws Exception {
+    final String grantUserName = name.getMethodName();
+    User grantUser = User.createUserForTesting(conf, grantUserName, new String[] {});
+    String namespace = name.getMethodName();
+    TableName table = TableName.valueOf(namespace, "t1");
+    String snapshot = namespace + "t1";
+
+    TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table);
+    admin.snapshot(snapshot, table);
+    TestHDFSAclHelper.grantOnTable(TEST_UTIL, grantUserName, table, READ);
+    TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot, 6);
+
+    // HFileCleaner will not delete archive table directory even if it's a empty directory
+    Path archiveTableDir = HFileArchiveUtil.getTableArchivePath(rootDir, table);
+    HFileCleaner cleaner = TEST_UTIL.getHBaseCluster().getMaster().getHFileCleaner();
+    cleaner.choreForTesting();
+    assertTrue(fs.exists(archiveTableDir));
+
+    // delete table and grant user can scan snapshot
+    admin.disableTable(table);
+    admin.deleteTable(table);
+    TestHDFSAclHelper.canUserScanSnapshot(TEST_UTIL, grantUser, snapshot, 6);
+
+    // Check SnapshotScannerHDFSAclHelper method
+    assertTrue(SnapshotScannerHDFSAclHelper.isArchiveTableDir(archiveTableDir));
+    assertTrue(SnapshotScannerHDFSAclHelper.isArchiveNamespaceDir(archiveTableDir.getParent()));
+    assertTrue(
+      SnapshotScannerHDFSAclHelper.isArchiveDataDir(archiveTableDir.getParent().getParent()));
+    assertFalse(SnapshotScannerHDFSAclHelper
+        .isArchiveDataDir(archiveTableDir.getParent().getParent().getParent()));
+  }
+
   @Test
   public void testGrantMobTable() throws Exception {
     final String grantUserName = name.getMethodName();