HADOOP-17227. Marker Tool tuning

* move from -expect to -min and -max; easier for CLI testing. Plus works
* in -nonauth mode, even when policy == keep, files not in an auth path
  count as failure.
* bucket-info option also prints out the authoritative path, so you have
  more idea what is happening
* reporting of command failure more informative

The reason for change #2 is a workflow where you want to audit a dir, even
though you are in keep mode, and you don't have any auth path. You'd expect
-nonauth to say "no auth path", but instead it treats the whole dir as
auth.

Change-Id: Ib310e321e5862957fbd92bebfade93231f92b16f

Author: steveloughran

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3GuardTool.java

@@ -1463,9 +1463,12 @@ public int run(String[] args, PrintStream out)
     private void processMarkerOption(final PrintStream out,
         final S3AFileSystem fs,
         final String marker) {
+      println(out, "%nSecurity");
       DirectoryPolicy markerPolicy = fs.getDirectoryMarkerPolicy();
       String desc = markerPolicy.describe();
-      println(out, "%nThe directory marker policy is \"%s\"%n", desc);
+      println(out, "\tThe directory marker policy is \"%s\"%n", desc);
+      printOption(out, "\tAuthoritative paths",
+          AUTHORITATIVE_PATH, "");
 
       DirectoryPolicy.MarkerPolicy mp = markerPolicy.getMarkerPolicy();
 

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/tools/MarkerTool.java

@@ -53,6 +53,7 @@
 import org.apache.hadoop.fs.s3a.UnknownStoreException;
 import org.apache.hadoop.fs.s3a.impl.DirMarkerTracker;
 import org.apache.hadoop.fs.s3a.impl.DirectoryPolicy;
+import org.apache.hadoop.fs.s3a.impl.DirectoryPolicyImpl;
 import org.apache.hadoop.fs.s3a.impl.StoreContext;
 import org.apache.hadoop.fs.s3a.s3guard.S3GuardTool;
 import org.apache.hadoop.fs.shell.CommandFormat;
@@ -113,9 +114,14 @@ public final class MarkerTool extends S3GuardTool {
   public static final String CLEAN = "-" + OPT_CLEAN;
 
   /**
-   * Expected number of markers to find: {@value}.
+   * Min number of markers to find: {@value}.
    */
-  public static final String OPT_EXPECTED = "expected";
+  public static final String OPT_MIN = "min";
+
+  /**
+   * Max number of markers to find: {@value}.
+   */
+  public static final String OPT_MAX = "max";
 
   /**
    * Name of a file to save the list of markers to: {@value}.
@@ -151,13 +157,21 @@ public final class MarkerTool extends S3GuardTool {
   public static final int UNLIMITED_LISTING = 0;
 
 
+  /**
+   * Constant to use when there is no minimum number of
+   * markers: {@value}.
+   */
+  public static final int UNLIMITED_MIN_MARKERS = -1;
+
+
   /**
    * Usage string: {@value}.
    */
   private static final String USAGE = MARKERS
       + " (-" + OPT_AUDIT
       + " | -" + OPT_CLEAN + ")"
-      + " [-" + OPT_EXPECTED + " <count>]"
+      + " [-" + OPT_MIN + " <count>]"
+      + " [-" + OPT_MAX + " <count>]"
       + " [-" + OPT_OUT + " <filename>]"
       + " [-" + OPT_LIMIT + " <limit>]"
       + " [-" + OPT_NONAUTH + "]"
@@ -195,7 +209,8 @@ public MarkerTool(final Configuration conf) {
         VERBOSE,
         OPT_NONAUTH);
     CommandFormat format = getCommandFormat();
-    format.addOptionWithValue(OPT_EXPECTED);
+    format.addOptionWithValue(OPT_MIN);
+    format.addOptionWithValue(OPT_MAX);
     format.addOptionWithValue(OPT_LIMIT);
     format.addOptionWithValue(OPT_OUT);
   }
@@ -231,8 +246,7 @@ public int run(final String[] args, final PrintStream stream)
     if (parsedArgs.size() != 1) {
       errorln(getUsage());
       println(out, "Supplied arguments: ["
-          + parsedArgs.stream()
-          .collect(Collectors.joining(", "))
+          + String.join(", ", parsedArgs)
           + "]");
       throw new ExitUtil.ExitException(EXIT_USAGE,
           String.format(E_ARGUMENTS, parsedArgs.size()));
@@ -241,12 +255,11 @@ public int run(final String[] args, final PrintStream stream)
     CommandFormat command = getCommandFormat();
     verbose = command.getOpt(VERBOSE);
 
-    // How many markers are expected?
-    int expected = 0;
-    String value = command.getOptValue(OPT_EXPECTED);
-    if (value != null && !value.isEmpty()) {
-      expected = Integer.parseInt(value);
-    }
+    // minimum number of markers expected
+    int expectedMin = getOptValue(OPT_MIN, 0);
+    // max number of markers allowed
+    int expectedMax = getOptValue(OPT_MAX, 0);
+
 
     // determine the action
     boolean audit = command.getOpt(OPT_AUDIT);
@@ -258,11 +271,7 @@ public int run(final String[] args, final PrintStream stream)
       throw new ExitUtil.ExitException(EXIT_USAGE,
           "Exactly one of " + AUDIT + " and " + CLEAN);
     }
-    int limit = UNLIMITED_LISTING;
-    value = command.getOptValue(OPT_LIMIT);
-    if (value != null && !value.isEmpty()) {
-      limit = Integer.parseInt(value);
-    }
+    int limit = getOptValue(OPT_LIMIT, UNLIMITED_LISTING);
     final String dir = parsedArgs.get(0);
     Path path = new Path(dir);
     URI uri = path.toUri();
@@ -275,7 +284,8 @@ public int run(final String[] args, final PrintStream stream)
         fs,
         path,
         clean,
-        expected,
+        expectedMin,
+        expectedMax,
         limit,
         command.getOpt(OPT_NONAUTH));
     if (verbose) {
@@ -300,16 +310,40 @@ public int run(final String[] args, final PrintStream stream)
         IOUtils.writeLines(surplus, "\n", writer);
       }
     }
-    return result.exitCode;
+
+    return result.finish();
+  }
+
+  /**
+   * Get the value of an option, or the default if the option
+   * is unset/empty.
+   * @param option option key
+   * @param defVal default
+   * @return the value to use
+   */
+  private int getOptValue(String option, int defVal) {
+    CommandFormat command = getCommandFormat();
+    String value = command.getOptValue(option);
+    if (value != null && !value.isEmpty()) {
+      try {
+        return  Integer.parseInt(value);
+      } catch (NumberFormatException e) {
+        throw new ExitUtil.ExitException(EXIT_USAGE,
+            String.format("Argument for %s is not a number: %s", option, value));
+      }
+    } else {
+      return defVal;
+    }
   }
 
   /**
    * Execute the scan/purge.
    * @param sourceFS source FS; must be or wrap an S3A FS.
    * @param path path to scan.
    * @param doPurge purge?
-   * @param expectedMarkerCount expected marker count
-   * @param limit limit of files to scan; -1 for 'unlimited'
+   * @param minMarkerCount min marker count (ignored on purge)
+   * @param maxMarkerCount max marker count (ignored on purge)
+   * @param limit limit of files to scan; 0 for 'unlimited'
    * @param nonAuth consider only markers in nonauth paths as errors
    * @return scan+purge result.
    * @throws IOException failure
@@ -319,7 +353,8 @@ ScanResult execute(
       final FileSystem sourceFS,
       final Path path,
       final boolean doPurge,
-      final int expectedMarkerCount,
+      final int minMarkerCount,
+      final int maxMarkerCount,
       final int limit,
       final boolean nonAuth)
       throws IOException {
@@ -360,10 +395,19 @@ ScanResult execute(
     }
 
     // the default filter policy is that all entries should be deleted
-    DirectoryPolicy filterPolicy = nonAuth
-        ? activePolicy
-        : null;
-    ScanResult result = scan(target, doPurge, expectedMarkerCount, limit,
+    DirectoryPolicy filterPolicy;
+    if (nonAuth) {
+      filterPolicy = new DirectoryPolicyImpl(
+          DirectoryPolicy.MarkerPolicy.Authoritative,
+          fs::allowAuthoritative);
+    } else {
+      filterPolicy = null;
+    }
+    ScanResult result = scan(target,
+        doPurge,
+        maxMarkerCount,
+        minMarkerCount,
+        limit,
         filterPolicy);
     return result;
   }
@@ -378,6 +422,22 @@ public static final class ScanResult {
      */
     private int exitCode;
 
+    /**
+     * Text to include if raising an exception.
+     */
+    private String exitText = "";
+
+    /**
+     * Count of all markers found.
+     */
+    private int totalMarkerCount;
+
+    /**
+     * Count of all markers found after excluding
+     * any from a [-nonauth] qualification;
+     */
+    private int filteredMarkerCount;
+
     /**
      * The tracker.
      */
@@ -395,6 +455,9 @@ private ScanResult() {
     public String toString() {
       return "ScanResult{" +
           "exitCode=" + exitCode +
+          ", exitText=" + exitText +
+          ", totalMarkerCount=" + totalMarkerCount +
+          ", filteredMarkerCount=" + filteredMarkerCount +
           ", tracker=" + tracker +
           ", purgeSummary=" + purgeSummary +
           '}';
@@ -414,13 +477,34 @@ public DirMarkerTracker getTracker() {
     public MarkerPurgeSummary getPurgeSummary() {
       return purgeSummary;
     }
+
+    public int getTotalMarkerCount() {
+      return totalMarkerCount;
+    }
+
+    public int getFilteredMarkerCount() {
+      return filteredMarkerCount;
+    }
+
+    /**
+     * Throw an exception if the exit code is non-zero.
+     * @return 0 if everything is good.
+     * @throws ExitUtil.ExitException if code != 0
+     */
+    public int finish() throws ExitUtil.ExitException {
+      if (exitCode != 0) {
+        throw new ExitUtil.ExitException(exitCode, exitText);
+      }
+      return 0;
+    }
   }
 
   /**
    * Do the scan/purge.
    * @param path path to scan.
-   * @param clean purge?
-   * @param expectedMarkerCount expected marker count
+   * @param doPurge purge rather than just scan/audit?
+   * @param minMarkerCount min marker count (ignored on purge)
+   * @param maxMarkerCount max marker count (ignored on purge)
    * @param limit limit of files to scan; 0 for 'unlimited'
    * @param filterPolicy filter policy on a nonauth scan; may be null
    * @return result.
@@ -430,8 +514,9 @@ public MarkerPurgeSummary getPurgeSummary() {
   @Retries.RetryTranslated
   private ScanResult scan(
       final Path path,
-      final boolean clean,
-      final int expectedMarkerCount,
+      final boolean doPurge,
+      final int minMarkerCount,
+      final int maxMarkerCount,
       final int limit,
       final DirectoryPolicy filterPolicy)
       throws IOException, ExitUtil.ExitException {
@@ -458,13 +543,16 @@ private ScanResult scan(
         = tracker.getSurplusMarkers();
     Map<Path, DirMarkerTracker.Marker> leafMarkers
         = tracker.getLeafMarkers();
-    int surplus = surplusMarkers.size();
-    if (surplus == 0) {
+    // determine marker count
+    int markerCount = surplusMarkers.size();
+    result.totalMarkerCount = markerCount;
+    result.filteredMarkerCount = markerCount;
+    if (markerCount == 0) {
       println(out, "No surplus directory markers were found under %s", path);
     } else {
       println(out, "Found %d surplus directory marker%s under %s",
-          surplus,
-          suffix(surplus),
+          markerCount,
+          suffix(markerCount),
           path);
 
       for (Path markers : surplusMarkers.keySet()) {
@@ -482,9 +570,9 @@ private ScanResult scan(
       println(out, "These are required to indicate empty directories");
     }
 
-    if (clean) {
+    if (doPurge) {
       // clean: remove the markers, do not worry about their
-      // presence when reporting success/failiure
+      // presence when reporting success/failure
       int deletePageSize = storeContext.getConfiguration()
           .getInt(BULK_DELETE_PAGE_SIZE,
               BULK_DELETE_PAGE_SIZE_DEFAULT);
@@ -503,28 +591,41 @@ private ScanResult scan(
           allowed.forEach(p -> println(out, p.toString()));
         }
         // recalculate the marker size
-        surplus = surplusMarkers.size();
+        markerCount = surplusMarkers.size();
+        result.filteredMarkerCount = markerCount;
       }
-      if (surplus > expectedMarkerCount) {
+      if (markerCount < minMarkerCount || markerCount > maxMarkerCount) {
         // failure
-        if (expectedMarkerCount > 0) {
-          println(out, "Expected %d marker%s", expectedMarkerCount,
-              suffix(surplus));
-        }
-        println(out, "Surplus markers were found -failing audit");
-
-        result.exitCode = EXIT_NOT_ACCEPTABLE;
+        return failScan(result, EXIT_NOT_ACCEPTABLE,
+            "Marker count %d out of range "
+                  + "[%d - %d]",
+              markerCount, minMarkerCount, maxMarkerCount);
       }
     }
 
     // now one little check for whether a limit was reached.
     if (!completed) {
-      println(out, "Listing limit reached before completing the scan");
-      result.exitCode = EXIT_INTERRUPTED;
+      failScan(result, EXIT_INTERRUPTED,
+          "Listing limit (%d) reached before completing the scan", limit);
     }
     return result;
   }
 
+  /**
+   * Fail the scan; print the formatted error and update the result.
+   * @param result result to update
+   * @param code Exit code
+   * @param message Error message
+   * @param args arguments for the error message
+   * @return scan result
+   */
+  private ScanResult failScan(ScanResult result, int code, String message, Object...args) {
+    String text = String.format(message, args);
+    result.exitCode = code;
+    result.exitText = text;
+    return result;
+  }
+
   /**
    * Suffix for plurals.
    * @param size size to generate a suffix for
@@ -702,7 +803,8 @@ public void setVerbose(final boolean verbose) {
    * @param sourceFS filesystem to use
    * @param path path to scan
    * @param doPurge should markers be purged
-   * @param expectedMarkers number of markers expected
+   * @param minMarkerCount min marker count (ignored on purge)
+   * @param maxMarkerCount max marker count (ignored on purge)
    * @param limit limit of files to scan; -1 for 'unlimited'
    * @param nonAuth only use nonauth path count for failure rules
    * @return the result
@@ -712,12 +814,14 @@ public static MarkerTool.ScanResult execMarkerTool(
       final FileSystem sourceFS,
       final Path path,
       final boolean doPurge,
-      final int expectedMarkers,
-      final int limit, boolean nonAuth) throws IOException {
+      final int minMarkerCount,
+      final int maxMarkerCount,
+      final int limit,
+      boolean nonAuth) throws IOException {
     MarkerTool tool = new MarkerTool(sourceFS.getConf());
     tool.setVerbose(LOG.isDebugEnabled());
 
     return tool.execute(sourceFS, path, doPurge,
-        expectedMarkers, limit, nonAuth);
+        minMarkerCount, maxMarkerCount, limit, nonAuth);
   }
 }