From e08333bfdf35b11d25bceb2331932a67304d98c6 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Tue, 13 Aug 2024 14:15:10 +0900 Subject: [PATCH] Ignore invalid configurations when migrating legacy mirrors and credentials (#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. --- .../mirror/MirroringMigrationServiceTest.java | 32 +++++++++++++- .../mirror/MirroringMigrationService.java | 42 ++++++++++--------- 2 files changed, 53 insertions(+), 21 deletions(-) diff --git a/server-mirror-git/src/test/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationServiceTest.java b/server-mirror-git/src/test/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationServiceTest.java index 8fdea5bda..8b887b216 100644 --- a/server-mirror-git/src/test/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationServiceTest.java +++ b/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>> 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", diff --git a/server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java b/server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java index 79652a4b4..dab98cbfb 100644 --- a/server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java +++ b/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> entries = repository.find(Revision.HEAD, targetDirectory + "**") .get(); - final List> changes = entries.keySet().stream().map(Change::ofRemoval) - .collect(toImmutableList()); - final Command 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> changes = entries.keySet().stream().map(Change::ofRemoval) + .collect(toImmutableList()); + + final Command 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++;