Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore invalid configurations when migrating legacy mirrors and credentials #1007

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Aug 13, 2024

Motivation:

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

} catch (Exception ex) {
final MirrorMigrationException mirrorException = new MirrorMigrationException(
"Failed to migrate mirrors and credentials. Rollback to the legacy configurations", ex);
try {
rollbackMigration();

However, the rollback also could fail if there is no migration on a project. entries may be empty which results in raising RedundantChangeException.
final Map<String, Entry<?>> entries = repository.find(Revision.HEAD, targetDirectory + "**")
.get();

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.

…ntials

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, I found out that the rollback also could fail if there is
no migration on a project. `entries` may be empty that 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
  migration the remaining projects.
- Try to rollback if the new migration directory is not empty.

Result:

- The server status is correctly reset to `WRITABLE` when a mirror
  migration job fails.
@ikhoon ikhoon added the defect label Aug 13, 2024
@ikhoon ikhoon added this to the 0.67.3 milestone Aug 13, 2024
@minwoox
Copy link
Member

minwoox commented Aug 13, 2024

return "mirror-" + projectName + '-' + mirror.get("localRepo").asText();

You will fix this in a different PR?

@ikhoon
Copy link
Contributor Author

ikhoon commented Aug 13, 2024

A mirror configuration that does not have localRepo will not perform correctly. So I let the configuration be ignored from migration. The exception is caught by

} 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);
}

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 👍 👍 👍

@ikhoon ikhoon merged commit e08333b into line:main Aug 13, 2024
10 checks passed
@ikhoon ikhoon deleted the mirror-migration-error-handling branch August 13, 2024 05:15
ikhoon added a commit to ikhoon/centraldogma that referenced this pull request Aug 13, 2024
…ntials (line#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants