Skip to content

Commit

Permalink
Ignore invalid configurations when migrating legacy mirrors and crede…
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@ikhoon
ikhoon committed Aug 13, 2024
1 parent ada1637 commit d936287
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 21 deletions.
32 changes: 30 additions & 2 deletions .../java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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" +
Expand Down Expand Up @@ -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 =
'{' +
Expand Down Expand Up @@ -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();
Expand All @@ -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()
Expand Down Expand Up @@ -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",
Expand Down
42 changes: 23 additions & 19 deletions ...main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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++;
Expand Down

0 comments on commit d936287

Please sign in to comment.