HDDS-15609. Legacy SCM Finalize command should become a no-op (#10543)

Author: sodonnel

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMClientProtocolServer.java

@@ -32,7 +32,6 @@
 import static org.apache.hadoop.ozone.upgrade.UpgradeFinalization.FINALIZATION_DONE_MSG;
 import static org.apache.hadoop.ozone.upgrade.UpgradeFinalization.FINALIZATION_REQUIRED_MSG;
 import static org.apache.hadoop.ozone.upgrade.UpgradeFinalization.Status.ALREADY_FINALIZED;
-import static org.apache.hadoop.ozone.upgrade.UpgradeFinalization.Status.STARTING_FINALIZATION;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
@@ -1150,15 +1149,19 @@ public ReplicationManagerReport getReplicationManagerReport() {
     return scm.getReplicationManager().getContainerReport();
   }
 
+  /*
+   * This command is deprecated and is retained for backward compatibility. It no longer finalizes SCM
+   * as the process is driven from OM which will trigger the SCM finalize process.
+   */
   @Override
   @Deprecated
-  public StatusAndMessages finalizeScmUpgrade(String upgradeClientID) throws
-      IOException {
-    if (!scm.getVersionManager().needsFinalization()) {
-      return new StatusAndMessages(ALREADY_FINALIZED, Collections.emptyList());
-    }
-    finalizeUpgrade();
-    return new StatusAndMessages(STARTING_FINALIZATION, Collections.emptyList());
+  public StatusAndMessages finalizeScmUpgrade(String upgradeClientID) {
+    // This command is kept only for legacy upgrade scripts which would have first finalized SCM and then
+    // made a call to OM to finalize it. The new flow, is that a single call to OM triggers the finalization process.
+    // The legacy OM command now calls the new one and correctly starts the flow, so this command has become a
+    // noop. Regardless of whether SCM is finalized or not, we return ALREADY_FINALIZED to allow any scripts to move
+    // on and call OM to start the process.
+    return new StatusAndMessages(ALREADY_FINALIZED, Collections.emptyList());
   }
 
   @Override

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMClientProtocolServer.java

@@ -19,12 +19,15 @@
 
 import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSED;
 import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_READONLY_ADMINISTRATORS;
+import static org.apache.hadoop.ozone.upgrade.UpgradeFinalization.Status.ALREADY_FINALIZED;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import java.io.File;
@@ -48,11 +51,15 @@
 import org.apache.hadoop.hdds.scm.pipeline.PipelineID;
 import org.apache.hadoop.hdds.scm.protocol.StorageContainerLocationProtocolServerSideTranslatorPB;
 import org.apache.hadoop.hdds.scm.safemode.SCMSafeModeManager;
+import org.apache.hadoop.hdds.scm.server.upgrade.FinalizationManager;
+import org.apache.hadoop.hdds.scm.server.upgrade.ScmVersionManager;
 import org.apache.hadoop.hdds.utils.ProtocolMessageMetrics;
 import org.apache.hadoop.ozone.container.common.SCMTestUtils;
+import org.apache.hadoop.ozone.upgrade.UpgradeFinalization.StatusAndMessages;
 import org.apache.hadoop.security.AccessControlException;
 import org.apache.hadoop.security.UserGroupInformation;
-import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
@@ -62,17 +69,16 @@
  * servicing commands from the scm client.
  */
 public class TestSCMClientProtocolServer {
-  private SCMClientProtocolServer server;
-  private StorageContainerManager scm;
-  private StorageContainerLocationProtocolServerSideTranslatorPB service;
-  private SCMSafeModeManager mockSafeModeManager;
-
-  @BeforeEach
-  void setUp(@TempDir File testDir) throws Exception {
+  private static SCMClientProtocolServer server;
+  private static StorageContainerManager scm;
+  private static StorageContainerLocationProtocolServerSideTranslatorPB service;
+  private static SCMSafeModeManager mockSafeModeManager;
+  
+  @BeforeAll
+  static void setUp(@TempDir File testDir) throws Exception {
     OzoneConfiguration config = SCMTestUtils.getConf(testDir);
 
     mockSafeModeManager = mock(SCMSafeModeManager.class);
-    when(mockSafeModeManager.getInSafeMode()).thenReturn(false);
 
     SCMConfigurator configurator = new SCMConfigurator();
     configurator.setSCMHAManager(SCMHAManagerStub.getInstance(true));
@@ -87,8 +93,13 @@ void setUp(@TempDir File testDir) throws Exception {
         scm, mock(ProtocolMessageMetrics.class));
   }
 
-  @AfterEach
-  public void tearDown() throws Exception {
+  @BeforeEach
+  void setUp() {
+    when(mockSafeModeManager.getInSafeMode()).thenReturn(false);
+  }
+
+  @AfterAll
+  public static void tearDown() throws Exception {
     if (scm != null) {
       scm.stop();
       scm.join();
@@ -181,6 +192,47 @@ private StorageContainerManager mockStorageContainerManager() {
     return storageContainerManager;
   }
 
+  @Test
+  public void testLegacyFinalizeScmUpgradeAlreadyFinalized() throws Exception {
+    FinalizationManager mockFinalizationManager = mock(FinalizationManager.class);
+    SCMClientProtocolServer testServer = serverWithMockFinalization(false, mockFinalizationManager);
+    try {
+      StatusAndMessages result = testServer.finalizeScmUpgrade("testClientID");
+      assertEquals(ALREADY_FINALIZED, result.status());
+      assertTrue(result.msgs().isEmpty());
+      verify(mockFinalizationManager, never()).finalizeUpgrade();
+    } finally {
+      testServer.stop();
+    }
+  }
+
+  @Test
+  public void testLegacyFinalizeScmUpgradeFinalizationRequired() throws Exception {
+    FinalizationManager mockFinalizationManager = mock(FinalizationManager.class);
+    SCMClientProtocolServer testServer = serverWithMockFinalization(true, mockFinalizationManager);
+    try {
+      StatusAndMessages result = testServer.finalizeScmUpgrade("testClientID");
+      assertEquals(ALREADY_FINALIZED, result.status());
+      assertTrue(result.msgs().isEmpty());
+      verify(mockFinalizationManager, never()).finalizeUpgrade();
+    } finally {
+      testServer.stop();
+    }
+  }
+
+  private SCMClientProtocolServer serverWithMockFinalization(
+      boolean needsFinalization, FinalizationManager finalizationManager) throws IOException {
+    ScmVersionManager mockVersionManager = mock(ScmVersionManager.class);
+    when(mockVersionManager.needsFinalization()).thenReturn(needsFinalization);
+
+    StorageContainerManager mockScm = mockStorageContainerManager();
+    when(mockScm.getVersionManager()).thenReturn(mockVersionManager);
+    when(mockScm.getFinalizationManager()).thenReturn(finalizationManager);
+
+    return new SCMClientProtocolServer(
+        new OzoneConfiguration(), mockScm, mock(ReconfigurationHandler.class));
+  }
+
   @Test
   public void testQueryUpgradeStatus() throws Exception {
     HddsProtos.UpgradeStatus status = server.queryUpgradeStatus();