Ignore invalid configurations when migrating legacy mirrors and crede…

…ntials (#1007)

Motivation:

If an exception is raised while performing a mirror migration job, it
tries to roll back the migration.

https://github.com/line/centraldogma/blob/5912bf7dd52bda8c414ba1e25c9a1155f981b42d/server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java#L135-L139
However, the rollback also could fail if there is no migration on a
project. `entries` may be empty which results in raising
`RedundantChangeException`.

https://github.com/line/centraldogma/blob/5912bf7dd52bda8c414ba1e25c9a1155f981b42d/server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java#L266-L267

Initially, I thought it was better to stop the whole migration job
immediately to make the migration status atomic if an exception was
raised. However, that was a bad choice. If it fails, it remains in
`REPLICATION_ONLY` mode, which is difficult to fix manually.

Modification:

- Always recover the server status to `WRITABLE` even when a mirror
migration job completes exceptionally.
- Warn invalid mirror and credential configurations and continue to
migrate the remaining projects.
- Try to roll back if the new migration directory is not empty.

Result:

- The server status is correctly reset to `WRITABLE` when a mirror
migration job fails.

server-mirror-git/src/test/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationServiceTest.java

@@ -120,6 +120,22 @@ class MirroringMigrationServiceTest {
  " ]\n" +
  '}';
 
+ // An invalid mirror config that has no localRepo.
+ static final String INVALID_MIRROR =
+ "{\n" +
+ " \"type\": \"single\",\n" +
+ " \"enabled\": true,\n" +
+ " \"schedule\": \"0 * * * * ?\",\n" +
+ " \"direction\": \"REMOTE_TO_LOCAL\",\n" +
+ " \"localPath\": \"/\",\n" +
+ " \"remoteUri\": \"git+ssh://git.bar.com/foo.git/settings#release\",\n" +
+ " \"credentialId\": \"credential-1\",\n" +
+ " \"gitignore\": [\n" +
+ " \"/credential.txt\",\n" +
+ " \"private_dir\"\n" +
+ " ]\n" +
+ '}';
+
  // A mirror config with duplicate ID
  static final String REPO2_MIRROR =
  "{\n" +
@@ -179,6 +195,16 @@ class MirroringMigrationServiceTest {
  " \"password\": \"sesame\"" +
  '}';
 
+ // An invalid credential
+ static final String INVALID_CREDENTIAL =
+ '{' +
+ " \"hostnamePatterns\": [" +
+ " \".*.bar\\\\.com$\"" +
+ " ]," +
+ " \"username\": \"trustin\"," +
+ " \"password\": \"sesame\"" +
+ '}';
+
  // A credential with duplicate ID
  static final String ACCESS_TOKEN_CREDENTIAL =
  '{' +
@@ -221,7 +247,8 @@ void shouldMigrateMirrorsJson() throws Exception {
  final Project project = projectManager.get(TEST_PROJ);
 
  final String mirrorsJson =
- '[' + REPO0_MIRROR + ',' + REPO1_MIRROR + ',' + REPO2_MIRROR + ',' + REPO3_MIRROR + ']';
+ '[' + REPO0_MIRROR + ',' + REPO1_MIRROR + ',' + INVALID_MIRROR +
+ ',' + REPO2_MIRROR + ',' + REPO3_MIRROR + ']';
  project.metaRepo().commit(Revision.HEAD, System.currentTimeMillis(), Author.SYSTEM,
  "Create a new mirrors.json",
  Change.ofJsonUpsert(PATH_LEGACY_MIRRORS, mirrorsJson)).join();
@@ -233,6 +260,7 @@ void shouldMigrateMirrorsJson() throws Exception {
  .find(Revision.HEAD, "/mirrors/*.json")
  .join();
 
+ // The invalid mirror config should be ignored.
  assertThat(entries).hasSize(4);
  final Map<String, Map.Entry<String, Entry<?>>> mirrors =
  entries.entrySet().stream()
@@ -268,7 +296,7 @@ void shouldMigrateCredential() throws Exception {
  final Project project = projectManager.get(TEST_PROJ);
 
  final String credentialJson = '[' + PUBLIC_KEY_CREDENTIAL + ',' + PASSWORD_CREDENTIAL + ',' +
- ACCESS_TOKEN_CREDENTIAL + ']';
+ INVALID_CREDENTIAL + ',' + ACCESS_TOKEN_CREDENTIAL + ']';
 
  project.metaRepo().commit(Revision.HEAD, System.currentTimeMillis(), Author.SYSTEM,
  "Create a new credentials.json",

server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java

@@ -132,6 +132,8 @@ void migrate() throws Exception {
  }
  }
  logMigrationJob(numMigratedProjects);
+ logger.info("Mirrors and credentials migration has been completed. (took: {} ms.)",
+ stopwatch.elapsed().toMillis());
  } catch (Exception ex) {
  final MirrorMigrationException mirrorException = new MirrorMigrationException(
  "Failed to migrate mirrors and credentials. Rollback to the legacy configurations", ex);
@@ -142,14 +144,12 @@ void migrate() throws Exception {
  throw new MirrorMigrationException("Failed to rollback the mirror migration:", ex0);
  }
  throw mirrorException;
+ } finally {
+ // Exit read-only mode.
+ commandExecutor.execute(Command.updateServerStatus(ServerStatus.WRITABLE))
+ .get(1, TimeUnit.MINUTES);
  }
 
- // Exit read-only mode.
- commandExecutor.execute(Command.updateServerStatus(ServerStatus.WRITABLE))
- .get(1, TimeUnit.MINUTES);
- logger.info("Mirrors and credentials migration has been completed. (took: {} ms.)",
- stopwatch.elapsed().toMillis());
-
  shortWords = null;
  }
 
@@ -208,9 +208,9 @@ private boolean migrateMirrors(MetaRepository repository) throws Exception {
  try {
  migrateMirror(repository, (ObjectNode) mirror, mirrorIds, credentials);
  } catch (Exception e) {
+ // Log the error and continue to migrate the next mirror.
  logger.warn("Failed to migrate a mirror config: {} (project: {})", mirror,
  repository.parent().name(), e);
- throw e;
  }
  }
  // Back up the old mirrors.json file and don't use it anymore.
@@ -265,18 +265,22 @@ private void rollbackMigration(MetaRepository repository, String targetDirectory
  // Delete all files in the target directory
  final Map<String, Entry<?>> entries = repository.find(Revision.HEAD, targetDirectory + "**")
  .get();
- final List<Change<?>> changes = entries.keySet().stream().map(Change::ofRemoval)
- .collect(toImmutableList());
- final Command<CommitResult> command =
- Command.push(Author.SYSTEM, repository.parent().name(),
- repository.name(), Revision.HEAD,
- "Rollback the migration of " + targetDirectory, "",
- Markup.PLAINTEXT, changes);
- try {
- executeCommand(command);
- } catch (InterruptedException | ExecutionException | TimeoutException e) {
- throw new MirrorMigrationException("Failed to rollback the migration of " + targetDirectory, e);
+ if (!entries.isEmpty()) {
+ final List<Change<?>> changes = entries.keySet().stream().map(Change::ofRemoval)
+ .collect(toImmutableList());
+
+ final Command<CommitResult> command =
+ Command.push(Author.SYSTEM, repository.parent().name(),
+ repository.name(), Revision.HEAD,
+ "Rollback the migration of " + targetDirectory, "",
+ Markup.PLAINTEXT, changes);
+ try {
+ executeCommand(command);
+ } catch (InterruptedException | ExecutionException | TimeoutException e) {
+ throw new MirrorMigrationException("Failed to rollback the migration of " + targetDirectory, e);
+ }
  }
+
  // Revert the backup file to the original file if exists.
  final Entry<?> backup = repository.getOrNull(Revision.HEAD, backupFile).get();
  if (backup != null) {
@@ -332,9 +336,9 @@ private boolean migrateCredentials(MetaRepository repository) throws Exception {
  try {
  migrateCredential(repository, (ObjectNode) credential, credentialIds);
  } catch (Exception e) {
+ // Log the error and continue to migrate the next credential.
  logger.warn("Failed to migrate the credential config in project {}",
  repository.parent().name(), e);
- throw e;
  }
  }
  index++;