HDDS-11946. Require all ozone repair commands to support a --dry-run option (#7682)

Author: adoroszlai

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/repair/om/TestFSORepairTool.java

@@ -346,8 +346,8 @@ private int dryRun(String... args) {
 
   private int execute(boolean dryRun, String... args) {
     List<String> argList = new ArrayList<>(Arrays.asList("om", "fso-tree", "--db", dbPath));
-    if (!dryRun) {
-      argList.add("--repair");
+    if (dryRun) {
+      argList.add("--dry-run");
     }
     argList.addAll(Arrays.asList(args));
 

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneRepairShell.java

@@ -51,6 +51,7 @@ public class TestOzoneRepairShell {
   private GenericTestUtils.PrintStreamCapturer err;
   private static MiniOzoneCluster cluster = null;
   private static OzoneConfiguration conf = null;
+  private static String om;
 
   private static final String TRANSACTION_INFO_TABLE_TERM_INDEX_PATTERN = "([0-9]+#[0-9]+)";
 
@@ -59,6 +60,7 @@ public static void init() throws Exception {
     conf = new OzoneConfiguration();
     cluster = MiniOzoneCluster.newBuilder(conf).build();
     cluster.waitForClusterToBeReady();
+    om = conf.get(OZONE_OM_ADDRESS_KEY);
   }
 
   @BeforeEach
@@ -136,17 +138,21 @@ private String[] parseScanOutput(String output) {
   public void testQuotaRepair() throws Exception {
     CommandLine cmd = new OzoneRepair().getCmd();
 
-    int exitCode = cmd.execute("om", "quota", "status", "--service-host", conf.get(OZONE_OM_ADDRESS_KEY));
+    int exitCode = cmd.execute("om", "quota", "status", "--service-host", om);
     assertEquals(0, exitCode, err);
 
+    cmd.execute("om", "quota", "start", "--dry-run", "--service-host", om);
+    cmd.execute("om", "quota", "status", "--service-host", om);
+    assertThat(out.get()).doesNotContain("lastRun");
+
     exitCode = withTextFromSystemIn("y")
-        .execute(() -> cmd.execute("om", "quota", "start", "--service-host", conf.get(OZONE_OM_ADDRESS_KEY)));
+        .execute(() -> cmd.execute("om", "quota", "start", "--service-host", om));
     assertEquals(0, exitCode, err);
 
     GenericTestUtils.waitFor(() -> {
       out.reset();
       // verify quota trigger is completed having non-zero lastRunFinishedTime
-      cmd.execute("om", "quota", "status", "--service-host", conf.get(OZONE_OM_ADDRESS_KEY));
+      cmd.execute("om", "quota", "status", "--service-host", om);
       try {
         return out.get().contains("\"lastRunFinishedTime\":\"\"");
       } catch (Exception ex) {

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/ReadOnlyCommand.java

@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+package org.apache.hadoop.ozone.repair;
+
+/** Marker interface for repair subcommands that do not modify state. */
+public interface ReadOnlyCommand {
+  // marker
+}

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/RepairTool.java

@@ -25,6 +25,7 @@
 import java.util.concurrent.Callable;
 
 /** Parent class for all actionable repair commands. */
+@CommandLine.Command
 public abstract class RepairTool extends AbstractSubcommand implements Callable<Void> {
 
   private static final String WARNING_SYS_USER_MESSAGE =
@@ -35,12 +36,20 @@ public abstract class RepairTool extends AbstractSubcommand implements Callable<
       description = "Use this flag if you want to bypass the check in false-positive cases.")
   private boolean force;
 
+  @CommandLine.Option(names = {"--dry-run"},
+      defaultValue = "false",
+      fallbackValue = "true",
+      description = "Simulate repair, but do not make any changes")
+  private boolean dryRun;
+
   /** Hook method for subclasses for performing actual repair task. */
   protected abstract void execute() throws Exception;
 
   @Override
   public final Void call() throws Exception {
-    confirmUser();
+    if (!dryRun) {
+      confirmUser();
+    }
     execute();
     return null;
   }
@@ -65,6 +74,10 @@ protected boolean checkIfServiceIsRunning(String serviceName) {
     return false;
   }
 
+  protected boolean isDryRun() {
+    return dryRun;
+  }
+
   protected void info(String msg, Object... args) {
     out().println(formatMessage(msg, args));
   }
@@ -77,6 +90,9 @@ private String formatMessage(String msg, Object[] args) {
     if (args != null && args.length > 0) {
       msg = String.format(msg, args);
     }
+    if (isDryRun()) {
+      msg = "[dry run] " + msg;
+    }
     return msg;
   }
 

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/om/FSORepairTool.java

@@ -85,11 +85,6 @@ public class FSORepairTool extends RepairTool {
       description = "Path to OM RocksDB")
   private String omDBPath;
 
-  @CommandLine.Option(names = {"-r", "--repair"},
-      defaultValue = "false",
-      description = "Run in repair mode to move unreferenced files and directories to deleted tables.")
-  private boolean repair;
-
   @CommandLine.Option(names = {"-v", "--volume"},
       description = "Filter by volume name. Add '/' before the volume name.")
   private String volumeFilter;
@@ -99,19 +94,14 @@ public class FSORepairTool extends RepairTool {
   private String bucketFilter;
 
   @CommandLine.Option(names = {"--verbose"},
-      description = "Verbose output. Show all intermediate steps and deleted keys info.")
+      description = "Verbose output. Show all intermediate steps.")
   private boolean verbose;
 
   @Override
   public void execute() throws Exception {
     if (checkIfServiceIsRunning("OM")) {
       return;
     }
-    if (repair) {
-      info("FSO Repair Tool is running in repair mode");
-    } else {
-      info("FSO Repair Tool is running in debug mode");
-    }
     try {
       Impl repairTool = new Impl();
       repairTool.run();
@@ -274,18 +264,12 @@ private boolean checkIfSnapshotExistsForBucket(String volumeName, String bucketN
     }
 
     private void processBucket(OmVolumeArgs volume, OmBucketInfo bucketInfo) throws IOException {
-      info("Processing bucket: " + volume.getVolume() + "/" + bucketInfo.getBucketName());
       if (checkIfSnapshotExistsForBucket(volume.getVolume(), bucketInfo.getBucketName())) {
-        if (!repair) {
-          info(
-              "Snapshot detected in bucket '" + volume.getVolume() + "/" + bucketInfo.getBucketName() + "'. ");
-        } else {
-          info(
-              "Skipping repair for bucket '" + volume.getVolume() + "/" + bucketInfo.getBucketName() + "' " +
-                  "due to snapshot presence.");
-          return;
-        }
+        info("Skipping repair for bucket '" + volume.getVolume() + "/" + bucketInfo.getBucketName() + "' " +
+                "due to snapshot presence.");
+        return;
       }
+      info("Processing bucket: " + volume.getVolume() + "/" + bucketInfo.getBucketName());
       markReachableObjectsInBucket(volume, bucketInfo);
       handleUnreachableAndUnreferencedObjects(volume, bucketInfo);
     }
@@ -359,15 +343,10 @@ private void handleUnreachableAndUnreferencedObjects(OmVolumeArgs volume, OmBuck
 
           if (!isReachable(dirKey)) {
             if (!isDirectoryInDeletedDirTable(dirKey)) {
-              info("Found unreferenced directory: " + dirKey);
               unreferencedStats.addDir();
 
-              if (!repair) {
-                if (verbose) {
-                  info("Marking unreferenced directory " + dirKey + " for deletion.");
-                }
-              } else {
-                info("Deleting unreferenced directory " + dirKey);
+              info("Deleting unreferenced directory " + dirKey);
+              if (!isDryRun()) {
                 OmDirectoryInfo dirInfo = dirEntry.getValue();
                 markDirectoryForDeletion(volume.getVolume(), bucket.getBucketName(), dirKey, dirInfo);
               }
@@ -393,15 +372,10 @@ private void handleUnreachableAndUnreferencedObjects(OmVolumeArgs volume, OmBuck
           OmKeyInfo fileInfo = fileEntry.getValue();
           if (!isReachable(fileKey)) {
             if (!isFileKeyInDeletedTable(fileKey)) {
-              info("Found unreferenced file: " + fileKey);
               unreferencedStats.addFile(fileInfo.getDataSize());
 
-              if (!repair) {
-                if (verbose) {
-                  info("Marking unreferenced file " + fileKey + " for deletion." + fileKey);
-                }
-              } else {
-                info("Deleting unreferenced file " + fileKey);
+              info("Deleting unreferenced file " + fileKey);
+              if (!isDryRun()) {
                 markFileForDeletion(fileKey, fileInfo);
               }
             } else {

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/om/SnapshotChainRepair.java

@@ -76,11 +76,6 @@ public class SnapshotChainRepair extends RepairTool {
       description = "Path previous snapshotId to set for the given snapshot")
   private UUID pathPreviousSnapshotId;
 
-  @CommandLine.Option(names = {"--dry-run"},
-      required = true,
-      description = "To dry-run the command.", defaultValue = "true")
-  private boolean dryRun;
-
   @Override
   public void execute() throws Exception {
     if (checkIfServiceIsRunning("OM")) {
@@ -139,12 +134,13 @@ public void execute() throws Exception {
       snapshotInfo.setGlobalPreviousSnapshotId(globalPreviousSnapshotId);
       snapshotInfo.setPathPreviousSnapshotId(pathPreviousSnapshotId);
 
-      if (dryRun) {
-        info("SnapshotInfo would be updated to : %s", snapshotInfo);
-      } else {
-        byte[] snapshotInfoBytes = SnapshotInfo.getCodec().toPersistedFormat(snapshotInfo);
-        db.get()
-            .put(snapshotInfoCfh, StringCodec.get().toPersistedFormat(snapshotInfoTableKey), snapshotInfoBytes);
+      info("Updating SnapshotInfo to %s", snapshotInfo);
+
+      byte[] snapshotInfoBytes = SnapshotInfo.getCodec().toPersistedFormat(snapshotInfo);
+      byte[] persistedFormat = StringCodec.get().toPersistedFormat(snapshotInfoTableKey);
+
+      if (!isDryRun()) {
+        db.get().put(snapshotInfoCfh, persistedFormat, snapshotInfoBytes);
 
         info("Snapshot Info is updated to : %s",
             RocksDBUtils.getValue(db, snapshotInfoCfh, snapshotInfoTableKey, SnapshotInfo.getCodec()));

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/om/TransactionInfoRepair.java

@@ -89,12 +89,17 @@ public void execute() throws Exception {
       TransactionInfo transactionInfo = TransactionInfo.valueOf(highestTransactionTerm, highestTransactionIndex);
 
       byte[] transactionInfoBytes = TransactionInfo.getCodec().toPersistedFormat(transactionInfo);
-      db.get()
-          .put(transactionInfoCfh, StringCodec.get().toPersistedFormat(TRANSACTION_INFO_KEY), transactionInfoBytes);
+      byte[] key = StringCodec.get().toPersistedFormat(TRANSACTION_INFO_KEY);
 
-      info("The highest transaction info has been updated to: %s",
-          RocksDBUtils.getValue(db, transactionInfoCfh, TRANSACTION_INFO_KEY,
-              TransactionInfo.getCodec()).getTermIndex());
+      info("Updating transaction info to %s", transactionInfo.getTermIndex());
+
+      if (!isDryRun()) {
+        db.get().put(transactionInfoCfh, key, transactionInfoBytes);
+
+        info("The highest transaction info has been updated to: %s",
+            RocksDBUtils.getValue(db, transactionInfoCfh, TRANSACTION_INFO_KEY,
+                TransactionInfo.getCodec()).getTermIndex());
+      }
     } catch (RocksDBException exception) {
       error("Failed to update the RocksDB for the given path: %s", dbPath);
       error(

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/om/quota/QuotaStatus.java

@@ -24,6 +24,7 @@
 import java.util.concurrent.Callable;
 import org.apache.hadoop.hdds.cli.HddsVersionProvider;
 import org.apache.hadoop.ozone.om.protocol.OzoneManagerProtocol;
+import org.apache.hadoop.ozone.repair.ReadOnlyCommand;
 import picocli.CommandLine;
 
 /**
@@ -35,7 +36,7 @@
     mixinStandardHelpOptions = true,
     versionProvider = HddsVersionProvider.class
 )
-public class QuotaStatus implements Callable<Void> {
+public class QuotaStatus implements Callable<Void>, ReadOnlyCommand {
 
   @CommandLine.Option(
       names = {"--service-id", "--om-service-id"},
@@ -57,9 +58,9 @@ public class QuotaStatus implements Callable<Void> {
 
   @Override
   public Void call() throws Exception {
-    OzoneManagerProtocol ozoneManagerClient =
-        parent.createOmClient(omServiceId, omHost, false);
-    System.out.println(ozoneManagerClient.getQuotaRepairStatus());
+    try (OzoneManagerProtocol omClient = parent.createOmClient(omServiceId, omHost, false)) {
+      System.out.println(omClient.getQuotaRepairStatus());
+    }
     return null;
   }
 }

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/om/quota/QuotaTrigger.java

@@ -77,8 +77,10 @@ public void execute() throws Exception {
           bucketList.isEmpty()
               ? "all buckets"
               : ("buckets " + buckets));
-      omClient.startQuotaRepair(bucketList);
-      info(omClient.getQuotaRepairStatus());
+      if (!isDryRun()) {
+        omClient.startQuotaRepair(bucketList);
+        info(omClient.getQuotaRepairStatus());
+      }
     }
   }
 }

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/repair/scm/cert/RecoverSCMCertificate.java

@@ -194,29 +194,31 @@ private void storeCerts(X509Certificate scmCertificate,
     CertificateCodec certCodec =
         new CertificateCodec(securityConfig, SCMCertificateClient.COMPONENT_NAME);
 
-    info("Writing certs to path : %s", certCodec.getLocation());
-
     CertPath certPath = addRootCertInPath(scmCertificate, rootCertificate);
     CertPath rootCertPath = getRootCertPath(rootCertificate);
     String encodedCert = CertificateCodec.getPEMEncodedString(certPath);
     String certName = String.format(CERT_FILE_NAME_FORMAT,
         CAType.NONE.getFileNamePrefix() + scmCertificate.getSerialNumber());
-    certCodec.writeCertificate(certName, encodedCert);
+    writeCertificate(certCodec, certName, encodedCert);
 
     String rootCertName = String.format(CERT_FILE_NAME_FORMAT,
         CAType.SUBORDINATE.getFileNamePrefix() + rootCertificate.getSerialNumber());
     String encodedRootCert = CertificateCodec.getPEMEncodedString(rootCertPath);
-    certCodec.writeCertificate(rootCertName, encodedRootCert);
+    writeCertificate(certCodec, rootCertName, encodedRootCert);
 
-    certCodec.writeCertificate(certCodec.getLocation().toAbsolutePath(),
-        securityConfig.getCertificateFileName(), encodedCert);
+    writeCertificate(certCodec, securityConfig.getCertificateFileName(), encodedCert);
 
     if (isRootCA) {
       CertificateCodec rootCertCodec =
           new CertificateCodec(securityConfig, OzoneConsts.SCM_ROOT_CA_COMPONENT_NAME);
-      info("Writing root certs to path : %s", rootCertCodec.getLocation());
-      rootCertCodec.writeCertificate(rootCertCodec.getLocation().toAbsolutePath(),
-          securityConfig.getCertificateFileName(), encodedRootCert);
+      writeCertificate(rootCertCodec, securityConfig.getCertificateFileName(), encodedRootCert);
+    }
+  }
+
+  private void writeCertificate(CertificateCodec codec, String name, String encodedCert) throws IOException {
+    info("Writing cert %s to %s", name, codec.getLocation());
+    if (!isDryRun()) {
+      codec.writeCertificate(name, encodedCert);
     }
   }
 

hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/repair/TestOzoneRepair.java

@@ -28,6 +28,7 @@
 import java.io.ByteArrayOutputStream;
 import java.io.PrintStream;
 import java.util.List;
+import java.util.Map;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Arrays.asList;
@@ -68,6 +69,33 @@ public void reset() {
     System.setProperty("user.name", OLD_USER);
   }
 
+  /** All leaf subcommands should support {@code --dry-run},
+   * except if marked as {@link ReadOnlyCommand}. */
+  @Test
+  void subcommandsSupportDryRun() {
+    assertSubcommandOptionRecursively(new OzoneRepair().getCmd());
+  }
+
+  private static void assertSubcommandOptionRecursively(CommandLine cmd) {
+    Map<String, CommandLine> subcommands = cmd.getSubcommands();
+    if (subcommands.isEmpty()) {
+      // leaf command
+      CommandLine.Model.CommandSpec spec = cmd.getCommandSpec();
+      Object userObject = spec.userObject();
+      if (!(userObject instanceof ReadOnlyCommand)) {
+        assertThat(spec.optionsMap().keySet())
+            .as(() -> "'" + spec.qualifiedName() + "' defined by " + userObject.getClass()
+                + " should support --dry-run or implement " + ReadOnlyCommand.class)
+            .contains("--dry-run");
+      }
+    } else {
+      // parent command
+      for (CommandLine sub : subcommands.values()) {
+        assertSubcommandOptionRecursively(sub);
+      }
+    }
+  }
+
   @Test
   void testOzoneRepairWhenUserIsRemindedSystemUserAndDeclinesToProceed() throws Exception {
     OzoneRepair ozoneRepair = new OzoneRepair();