Support online NoteDb migration with a flag to Daemon

Change-Id: I58e73ae2b3dcefd3d459b3cc2c095f2028c1a59b

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java

@@ -46,6 +46,7 @@ import java.net.InetSocketAddress;
 import java.net.URI;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.BrokenBarrierException;
@@ -54,6 +55,7 @@ import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Stream;
 import org.apache.log4j.Level;
 import org.apache.log4j.Logger;
 import org.eclipse.jgit.lib.Config;
@@ -245,11 +247,17 @@ public class GerritServer implements AutoCloseable {
    *     servers. For on-disk servers, assumes that {@link #init} was previously called to
    *     initialize this directory.
    * @param testSysModule optional additional module to add to the system injector.
+   * @param additionalArgs additional command-line arguments for the daemon program; only allowed if
+   *     the test is not in-memory.
    * @return started server.
    * @throws Exception
    */
   public static GerritServer start(
-      Description desc, Config baseConfig, Path site, @Nullable Module testSysModule)
+      Description desc,
+      Config baseConfig,
+      Path site,
+      @Nullable Module testSysModule,
+      String... additionalArgs)
       throws Exception {
     checkArgument(site != null, "site is required (even for in-memory server");
     desc.checkValidAnnotations();
@@ -270,9 +278,10 @@ public class GerritServer implements AutoCloseable {
     daemon.setEnableSshd(desc.useSsh());
 
     if (desc.memory()) {
+      checkArgument(additionalArgs.length == 0, "cannot pass args to in-memory server");
       return startInMemory(desc, baseConfig, daemon);
     }
-    return startOnDisk(desc, site, daemon, serverStarted);
+    return startOnDisk(desc, site, daemon, serverStarted, additionalArgs);
   }
 
   private static GerritServer startInMemory(Description desc, Config baseConfig, Daemon daemon)
@@ -294,18 +303,25 @@ public class GerritServer implements AutoCloseable {
   }
 
   private static GerritServer startOnDisk(
-      Description desc, Path site, Daemon daemon, CyclicBarrier serverStarted) throws Exception {
+      Description desc,
+      Path site,
+      Daemon daemon,
+      CyclicBarrier serverStarted,
+      String[] additionalArgs)
+      throws Exception {
     checkNotNull(site);
     ExecutorService daemonService = Executors.newSingleThreadExecutor();
+    String[] args =
+        Stream.concat(
+                Stream.of(
+                    "-d", site.toString(), "--headless", "--console-log", "--show-stack-trace"),
+                Arrays.stream(additionalArgs))
+            .toArray(String[]::new);
     @SuppressWarnings("unused")
     Future<?> possiblyIgnoredError =
         daemonService.submit(
             () -> {
-              int rc =
-                  daemon.main(
-                      new String[] {
-                        "-d", site.toString(), "--headless", "--console-log", "--show-stack-trace",
-                      });
+              int rc = daemon.main(args);
               if (rc != 0) {
                 System.err.println("Failed to start Gerrit daemon");
                 serverStarted.reset();

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/StandaloneSiteTest.java

@@ -118,8 +118,9 @@ public abstract class StandaloneSiteTest {
     return startServer(null);
   }
 
-  protected ServerContext startServer(@Nullable Module testSysModule) throws Exception {
-    return new ServerContext(startImpl(testSysModule));
+  protected ServerContext startServer(@Nullable Module testSysModule, String... additionalArgs)
+      throws Exception {
+    return new ServerContext(startImpl(testSysModule, additionalArgs));
   }
 
   protected void assertServerStartupFails() throws Exception {
@@ -130,8 +131,10 @@ public abstract class StandaloneSiteTest {
     }
   }
 
-  private GerritServer startImpl(@Nullable Module testSysModule) throws Exception {
-    return GerritServer.start(serverDesc, baseConfig, sitePaths.site_path, testSysModule);
+  private GerritServer startImpl(@Nullable Module testSysModule, String... additionalArgs)
+      throws Exception {
+    return GerritServer.start(
+        serverDesc, baseConfig, sitePaths.site_path, testSysModule, additionalArgs);
   }
 
   protected static void runGerrit(String... args) throws Exception {

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/IndexUpgradeController.java

@@ -15,7 +15,6 @@
 package com.google.gerrit.acceptance.pgm;
 
 import static com.google.common.base.Preconditions.checkState;
-import static com.google.common.truth.Truth.assert_;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
@@ -26,7 +25,6 @@ import com.google.inject.Module;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.TimeUnit;
 
 class IndexUpgradeController implements OnlineUpgradeListener {
   @AutoValue
@@ -105,21 +103,7 @@ class IndexUpgradeController implements OnlineUpgradeListener {
 
   void runUpgrades() throws Exception {
     readyToStart.countDown();
-
-    // Wait with a timeout. Startup should happen quickly, but bugs preventing upgrading from
-    // starting might not be that uncommon, so we don't want to have to wait forever to discover
-    // them.
-    int timeoutSec = 60;
-    if (!started.await(timeoutSec, TimeUnit.SECONDS)) {
-      assert_()
-          .fail(
-              "%s/%s online upgrades started after %ss",
-              numExpected - started.getCount(), numExpected, timeoutSec);
-    }
-
-    // Wait with no timeout. Reindexing might be slow, and given that upgrading started
-    // successfully, it's unlikely there is a bug preventing it from tripping the finished latch
-    // eventually, even if it takes longer than we might guess.
+    started.await();
     finished.await();
   }
 

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java

@@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.index.GerritIndexStatus;
+import com.google.gerrit.server.index.change.ChangeIndexCollection;
 import com.google.gerrit.server.index.change.ChangeSchemaDefinitions;
 import com.google.gerrit.server.notedb.ConfigNotesMigration;
 import com.google.gerrit.server.notedb.NoteDbChangeState;
@@ -49,14 +50,15 @@ import org.junit.Before;
 import org.junit.Test;
 
 /**
- * Tests for offline {@code migrate-to-note-db} program.
+ * Tests for NoteDb migrations where the entry point is through a program, {@code
+ * migrate-to-note-db} or {@code daemon}.
  *
  * <p><strong>Note:</strong> These tests are very slow due to the repeated daemon startup. Prefer
  * adding tests to {@link com.google.gerrit.acceptance.server.notedb.OnlineNoteDbMigrationIT} if
  * possible.
  */
 @NoHttpd
-public class OfflineNoteDbMigrationIT extends StandaloneSiteTest {
+public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest {
   private StoredConfig gerritConfig;
 
   private Project.NameKey project;
@@ -145,6 +147,36 @@ public class OfflineNoteDbMigrationIT extends StandaloneSiteTest {
     assertThat(status.getReady(ChangeSchemaDefinitions.NAME, version)).isTrue();
   }
 
+  @Test
+  public void onlineMigrationViaDaemon() throws Exception {
+    assertNotesMigrationState(NotesMigrationState.REVIEW_DB);
+    int prevVersion = ChangeSchemaDefinitions.INSTANCE.getPrevious().getVersion();
+    int currVersion = ChangeSchemaDefinitions.INSTANCE.getLatest().getVersion();
+
+    // Before storing any changes, switch back to the previous version.
+    GerritIndexStatus status = new GerritIndexStatus(sitePaths);
+    status.setReady(ChangeSchemaDefinitions.NAME, currVersion, false);
+    status.setReady(ChangeSchemaDefinitions.NAME, prevVersion, true);
+    status.save();
+
+    setOnlineUpgradeConfig(false);
+    setUpOneChange();
+    setOnlineUpgradeConfig(true);
+
+    IndexUpgradeController u = new IndexUpgradeController(1);
+    try (ServerContext ctx = startServer(u.module(), "--migrate-to-note-db", "true")) {
+      ChangeIndexCollection indexes = ctx.getInjector().getInstance(ChangeIndexCollection.class);
+      assertThat(indexes.getSearchIndex().getSchema().getVersion()).isEqualTo(prevVersion);
+
+      // Index schema upgrades happen after NoteDb migration, so waiting for those to complete
+      // should be sufficient.
+      u.runUpgrades();
+
+      assertThat(indexes.getSearchIndex().getSchema().getVersion()).isEqualTo(currVersion);
+      assertNotesMigrationState(NotesMigrationState.NOTE_DB_UNFUSED);
+    }
+  }
+
   private void setUpOneChange() throws Exception {
     project = new Project.NameKey("project");
     try (ServerContext ctx = startServer()) {
@@ -175,4 +207,10 @@ public class OfflineNoteDbMigrationIT extends StandaloneSiteTest {
         .getInstance(Key.get(new TypeLiteral<SchemaFactory<ReviewDb>>() {}, ReviewDbFactory.class))
         .open();
   }
+
+  private void setOnlineUpgradeConfig(boolean enable) throws Exception {
+    gerritConfig.load();
+    gerritConfig.setBoolean("index", null, "onlineUpgrade", enable);
+    gerritConfig.save();
+  }
 }

gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java

@@ -75,6 +75,7 @@ import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier;
 import com.google.gerrit.server.mail.receive.MailReceiver;
 import com.google.gerrit.server.mail.send.SmtpEmailSender;
 import com.google.gerrit.server.mime.MimeUtil2Module;
+import com.google.gerrit.server.notedb.rebuild.OnlineNoteDbMigrator;
 import com.google.gerrit.server.patch.DiffExecutorModule;
 import com.google.gerrit.server.plugins.PluginGuiceEnvironment;
 import com.google.gerrit.server.plugins.PluginModule;
@@ -113,6 +114,7 @@ import java.util.List;
 import javax.servlet.http.HttpServletRequest;
 import org.eclipse.jgit.lib.Config;
 import org.kohsuke.args4j.Option;
+import org.kohsuke.args4j.spi.ExplicitBooleanOptionHandler;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -164,6 +166,13 @@ public class Daemon extends SiteProgram {
   @Option(name = "--stop-only", usage = "Stop the daemon", hidden = true)
   private boolean stopOnly;
 
+  @Option(
+    name = "--migrate-to-note-db",
+    usage = "(EXPERIMENTAL) Automatically migrate changes to NoteDb",
+    handler = ExplicitBooleanOptionHandler.class
+  )
+  private boolean migrateToNoteDb;
+
   private final LifecycleManager manager = new LifecycleManager();
   private Injector dbInjector;
   private Injector cfgInjector;
@@ -450,6 +459,9 @@ public class Daemon extends SiteProgram {
       modules.add(new ChangeCleanupRunner.Module());
     }
     modules.addAll(LibModuleLoader.loadModules(cfgInjector));
+    if (migrateToNoteDb) {
+      modules.add(new OnlineNoteDbMigrator.Module());
+    }
     if (testSysModule != null) {
       modules.add(testSysModule);
     }
@@ -463,7 +475,10 @@ public class Daemon extends SiteProgram {
     if (luceneModule != null) {
       return luceneModule;
     }
-    boolean onlineUpgrade = VersionManager.getOnlineUpgrade(config);
+    boolean onlineUpgrade =
+        VersionManager.getOnlineUpgrade(config)
+            // Schema upgrade is handled by OnlineNoteDbMigrator in this case.
+            && !migrateToNoteDb;
     switch (indexType) {
       case LUCENE:
         return onlineUpgrade

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java

@@ -57,6 +57,8 @@ import com.google.gerrit.server.notedb.NotesMigrationState;
 import com.google.gerrit.server.notedb.PrimaryStorageMigrator;
 import com.google.gerrit.server.notedb.RepoSequence;
 import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException;
+import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.gerrit.server.util.OneOffRequestContext;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Inject;
@@ -92,6 +94,7 @@ public class NoteDbMigrator implements AutoCloseable {
     private final SchemaFactory<ReviewDb> schemaFactory;
     private final GitRepositoryManager repoManager;
     private final AllProjectsName allProjects;
+    private final OneOffRequestContext requestContext;
     private final ChangeRebuilder rebuilder;
     private final WorkQueue workQueue;
     private final NotesMigration globalNotesMigration;
@@ -113,6 +116,7 @@ public class NoteDbMigrator implements AutoCloseable {
         SchemaFactory<ReviewDb> schemaFactory,
         GitRepositoryManager repoManager,
         AllProjectsName allProjects,
+        OneOffRequestContext requestContext,
         ChangeRebuilder rebuilder,
         WorkQueue workQueue,
         NotesMigration globalNotesMigration,
@@ -122,6 +126,7 @@ public class NoteDbMigrator implements AutoCloseable {
       this.schemaFactory = schemaFactory;
       this.repoManager = repoManager;
       this.allProjects = allProjects;
+      this.requestContext = requestContext;
       this.rebuilder = rebuilder;
       this.workQueue = workQueue;
       this.globalNotesMigration = globalNotesMigration;
@@ -261,6 +266,7 @@ public class NoteDbMigrator implements AutoCloseable {
           schemaFactory,
           repoManager,
           allProjects,
+          requestContext,
           rebuilder,
           globalNotesMigration,
           primaryStorageMigrator,
@@ -281,6 +287,7 @@ public class NoteDbMigrator implements AutoCloseable {
   private final SchemaFactory<ReviewDb> schemaFactory;
   private final GitRepositoryManager repoManager;
   private final AllProjectsName allProjects;
+  private final OneOffRequestContext requestContext;
   private final ChangeRebuilder rebuilder;
   private final NotesMigration globalNotesMigration;
   private final PrimaryStorageMigrator primaryStorageMigrator;
@@ -299,6 +306,7 @@ public class NoteDbMigrator implements AutoCloseable {
       SchemaFactory<ReviewDb> schemaFactory,
       GitRepositoryManager repoManager,
       AllProjectsName allProjects,
+      OneOffRequestContext requestContext,
       ChangeRebuilder rebuilder,
       NotesMigration globalNotesMigration,
       PrimaryStorageMigrator primaryStorageMigrator,
@@ -322,6 +330,7 @@ public class NoteDbMigrator implements AutoCloseable {
     this.rebuilder = rebuilder;
     this.repoManager = repoManager;
     this.allProjects = allProjects;
+    this.requestContext = requestContext;
     this.globalNotesMigration = globalNotesMigration;
     this.primaryStorageMigrator = primaryStorageMigrator;
     this.gerritConfig = new FileBasedConfig(sitePaths.gerrit_config.toFile(), FS.detect());
@@ -479,7 +488,7 @@ public class NoteDbMigrator implements AutoCloseable {
                     executor.submit(
                         () -> {
                           // TODO(dborowitz): Avoid reopening db if using a single thread.
-                          try (ReviewDb db = unwrapDb(schemaFactory.open())) {
+                          try (ManualRequestContext ctx = requestContext.open()) {
                             primaryStorageMigrator.migrateToNoteDbPrimary(id);
                             return true;
                           } catch (Exception e) {

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/OnlineNoteDbMigrator.java

@@ -0,0 +1,89 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed 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 com.google.gerrit.server.notedb.rebuild;
+
+import com.google.common.base.Stopwatch;
+import com.google.gerrit.extensions.events.LifecycleListener;
+import com.google.gerrit.lifecycle.LifecycleModule;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.index.OnlineUpgrader;
+import com.google.gerrit.server.index.VersionManager;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import java.util.concurrent.TimeUnit;
+import org.eclipse.jgit.lib.Config;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Singleton
+public class OnlineNoteDbMigrator implements LifecycleListener {
+  private static final Logger log = LoggerFactory.getLogger(OnlineNoteDbMigrator.class);
+
+  public static class Module extends LifecycleModule {
+    @Override
+    public void configure() {
+      listener().to(OnlineNoteDbMigrator.class);
+    }
+  }
+
+  private Provider<NoteDbMigrator.Builder> migratorBuilderProvider;
+  private final OnlineUpgrader indexUpgrader;
+  private final boolean upgradeIndex;
+
+  @Inject
+  OnlineNoteDbMigrator(
+      @GerritServerConfig Config cfg,
+      Provider<NoteDbMigrator.Builder> migratorBuilderProvider,
+      OnlineUpgrader indexUpgrader) {
+    this.migratorBuilderProvider = migratorBuilderProvider;
+    this.indexUpgrader = indexUpgrader;
+    this.upgradeIndex = VersionManager.getOnlineUpgrade(cfg);
+  }
+
+  @Override
+  public void start() {
+    Thread t = new Thread(this::migrate);
+    t.setDaemon(true);
+    t.setName(getClass().getSimpleName());
+    t.start();
+  }
+
+  private void migrate() {
+    log.info("Starting online NoteDb migration");
+    if (upgradeIndex) {
+      log.info("Online index schema upgrades will be deferred until NoteDb migration is complete");
+    }
+    Stopwatch sw = Stopwatch.createStarted();
+    // TODO(dborowitz): Tune threads, maybe expose a progress monitor somewhere.
+    try (NoteDbMigrator migrator = migratorBuilderProvider.get().build()) {
+      migrator.migrate();
+    } catch (Exception e) {
+      log.error("Error in online NoteDb migration", e);
+    }
+    log.info("Online NoteDb migration completed in {}s", sw.elapsed(TimeUnit.SECONDS));
+
+    if (upgradeIndex) {
+      log.info("Starting deferred index schema upgrades");
+      indexUpgrader.start();
+    }
+  }
+
+  @Override
+  public void stop() {
+    // Do nothing; upgrade process uses daemon threads and knows how to recover from failures on
+    // next attempt.
+  }
+}