-
Notifications
You must be signed in to change notification settings - Fork 118
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
Introduce new mirroring and credential settings format and REST API #880
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #880 +/- ##
============================================
+ Coverage 66.86% 70.19% +3.33%
- Complexity 3529 3820 +291
============================================
Files 370 382 +12
Lines 14531 15357 +826
Branches 1563 1643 +80
============================================
+ Hits 9716 10780 +1064
+ Misses 3936 3617 -319
- Partials 879 960 +81 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great all in all. 👍 👍 👍
server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/AbstractMirror.java
Show resolved
Hide resolved
...va/com/linecorp/centraldogma/server/internal/mirror/MirroringAndCredentialServiceV1Test.java
Outdated
Show resolved
Hide resolved
...va/com/linecorp/centraldogma/server/internal/mirror/MirroringAndCredentialServiceV1Test.java
Outdated
Show resolved
Hide resolved
...va/com/linecorp/centraldogma/server/internal/mirror/MirroringAndCredentialServiceV1Test.java
Outdated
Show resolved
Hide resolved
...va/com/linecorp/centraldogma/server/internal/mirror/MirroringAndCredentialServiceV1Test.java
Outdated
Show resolved
Hide resolved
...java/com/linecorp/centraldogma/server/internal/storage/repository/DefaultMetaRepository.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/linecorp/centraldogma/server/internal/storage/repository/RepositoryUri.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/linecorp/centraldogma/server/internal/storage/repository/RepositoryUri.java
Outdated
Show resolved
Hide resolved
...a/com/linecorp/centraldogma/server/internal/mirror/credential/PublicKeyMirrorCredential.java
Outdated
Show resolved
Hide resolved
...a/com/linecorp/centraldogma/server/internal/mirror/credential/PublicKeyMirrorCredential.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get a chance to look at the services, but overall looks nice. Left some questions 🙇
common/src/main/java/com/linecorp/centraldogma/internal/jsonpatch/AddOperation.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/linecorp/centraldogma/internal/jsonpatch/AddOperation.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java
Outdated
Show resolved
Hide resolved
final JsonNode credentialId = mirror.get("credentialId"); | ||
if (credentialId != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I have no idea how to prevent JSON upsert operation. We may need to stop replication just before rolling updates.
Instead of the current approach of updating from a Leader directly, would it be possible to issue a custom command (i.e. MigrateMirrorCommand
) ?
If we take a write lock while migrating, I guess there will be minimal issues regarding inconsistency.
...rc/main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java
Outdated
Show resolved
Hide resolved
This PR is now ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through the controller layer for now, looks good overall 👍
* | ||
* <p>Returns the mirror of the ID in the project mirror list. | ||
*/ | ||
@RequiresReadPermission(repository = Project.REPO_META) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that applying RequiresReadPermission
to both the class and method will apply both decorators. (So both READ
and WRITE
permissions are necessary for getMirror
). I wanted to make sure if this is intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought method annotation had higher precedence. If so, it needs to be fixed.
return createOrUpdate(projectName, credential, author, true); | ||
} | ||
|
||
private CompletableFuture<PushResultDto> createOrUpdate(String projectName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question) Is there a reason we separate create from update? (e.g. is it easier to express in the UI?) Given the distributed nature of CD, I think I usually prefer that we provide an upsert method instead which is why I'm asking. (e.g. the semantics of update
may not hold if two concurrent requests go through).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to implement a more restful API.
create
needsPOST
method and201 Created
status is returned.update
usesPUT
method to replace the whole content and 200 status will be returned instead.
Aside from the REST endpoint, the internal goes through the same code path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the REST endpoint, the internal goes through the same code path.
Understood. I personally I prefer simplicity but I think it's fine to keep as is then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update
is not necessary for this API, but a consistent convention for all REST endpoints would be better.
Users may get confused if only some resources provide the PUT operation.
As REST API continues to be added to CD, there will be a high possibility of using PUT in the future.
- POST is a non-idempotent operation but
PUT
is idempotent so retryable. - We may add more validations to
PUT
if some fields should be immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a consistent convention for all REST endpoints would be better.
I'm not strong on the definition/usefulness of REST. I just wanted to make clear the reasoning for adding this logic in case we need to revisit later.
common/src/main/java/com/linecorp/centraldogma/internal/api/v1/PushResultDto.java
Outdated
Show resolved
Hide resolved
private boolean wasMigrated() throws Exception { | ||
for (Project project : projectManager.list().values()) { | ||
final MetaRepository repository = project.metaRepo(); | ||
if (getMetaData(repository, PATH_LEGACY_MIRRORS_BACKUP) != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question on the null check here - am I understanding correctly that if a legacy mirrors.json
/credentials.json
doesn't exist for a project then no backup file will exist?
i.e. If there is a repository without mirroring configured, will this method always return false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if a legacy mirrors.json/credentials.json doesn't exist for a project then no backup file will exist?
Correct. Backup files won't be created if no mirror is configured.
Lines 164 to 167 in a80425c
final ArrayNode mirrors = getMetaData(repository, PATH_LEGACY_MIRRORS); | |
if (mirrors == null) { | |
return false; | |
} |
i.e. If there is a repository without mirroring configured, will this method always return false?
This method returns false
but nothing will be performed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method returns false but nothing will be performed.
I see. I just wanted to make sure the following lines won't be executed after a migration is successfully completed.
Lines 100 to 108 in f0184f0
// Enter read-only mode. | |
commandExecutor.execute(Command.updateServerStatus(ServerStatus.REPLICATION_ONLY)) | |
.get(1, TimeUnit.MINUTES); | |
logger.info("Starting Mirrors and credentials migration ..."); | |
if (commandExecutor instanceof ZooKeeperCommandExecutor) { | |
logger.debug("Waiting for 30 seconds to make sure that all cluster have been notified of the " + | |
"read-only mode ..."); | |
Thread.sleep(30000); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/mirror-migration-job.json
is added to check if a migration has been performed before.
/mirror-migration-job.json
will be written even if there are no mirror and credential configurations.
return createOrUpdate(projectName, credential, author, true); | ||
} | ||
|
||
private CompletableFuture<PushResultDto> createOrUpdate(String projectName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the REST endpoint, the internal goes through the same code path.
Understood. I personally I prefer simplicity but I think it's fine to keep as is then.
try { | ||
migrateMirror(repository, (ObjectNode) mirror, mirrorIds, credentials); | ||
} catch (Exception e) { | ||
logger.warn("Failed to migrate a mirror config: {} (project: {})", mirror, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question) Am I understanding correctly that even if a mirroring attempt fails (e.g. due to timeout), the migration may go through? If so, is manual intervention required?
e.g. If a timeout results in a failed mirror migration at this line, the server will still proceed to start up.
In general, I think if a logical failure occurs it may be better to just throw and handle ourselves. If a sporadic failure (timeout) occurs, we could just retry by restarting the server and triggering migrateMirrors
again (ideally progress won't be lost)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Let me log the exception and rethrow it.
} | ||
} | ||
// Back up the old mirrors.json file and don't use it anymore. | ||
rename(repository, PATH_LEGACY_MIRRORS, PATH_LEGACY_MIRRORS_BACKUP, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal) I'm curious if the old meta/[mirrors|credentials].json
will be visible to users in the new mirroring UI. If not, what do you think of doing the rename/backup migrations after a few versions to ensure stability?
I realize that changes to the new mirroring/credentials won't be reflected in the old mirrors (unless we pre-implement and release some logic). I still think it's better than nothing and provides us an escape hatch if something goes wrong in the new release (e.g. it may not necessarily be due to this feature, but an armeria bug may force us to revert versions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. It's a good idea, but it's not easy to apply. What if a bug is discovered a certain amount of time after the new mirroring configuration is exposed to users?
If an update has already been made in the new file, converting to the old file does not seem to be a good option. The migration itself should be verified through testing, and for bugs caused by external libraries, it would be better to revert the version only rather than reverting the all changes.
Speaking of the release strategy, what do you think of releasing a new version only for this change to minimize side effects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. It's a good idea, but it's not easy to apply. What if a bug is discovered a certain amount of time after the new mirroring configuration is exposed to users?
I see. I was thinking there is a higher probability that a bug is discovered right away rather than a long time. This comment also assumed that mirroring configurations are usually not updated often.
I understood we prefer the current approach - if a roll back to the previous version is needed, no mirrors will be executed.
private boolean wasMigrated() throws Exception { | ||
for (Project project : projectManager.list().values()) { | ||
final MetaRepository repository = project.metaRepo(); | ||
if (getMetaData(repository, PATH_LEGACY_MIRRORS_BACKUP) != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method returns false but nothing will be performed.
I see. I just wanted to make sure the following lines won't be executed after a migration is successfully completed.
Lines 100 to 108 in f0184f0
// Enter read-only mode. | |
commandExecutor.execute(Command.updateServerStatus(ServerStatus.REPLICATION_ONLY)) | |
.get(1, TimeUnit.MINUTES); | |
logger.info("Starting Mirrors and credentials migration ..."); | |
if (commandExecutor instanceof ZooKeeperCommandExecutor) { | |
logger.debug("Waiting for 30 seconds to make sure that all cluster have been notified of the " + | |
"read-only mode ..."); | |
Thread.sleep(30000); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through all of the files and looks good overall 👍 Just left a couple comments on whether migration can be done safely and we have a clear rollback plan. Other parts look good to me 👍
final Throwable peeled = Exceptions.peel(cause); | ||
if (peeled instanceof ReadOnlyException) { | ||
// The executor has stopped right after starting up. | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question to make sure I'm understanding correctly. Does this scenario occur because of the server status update? If a server doesn't have the necessary internal files but an exception isn't thrown here, could a CD server exist without the dogma
project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this scenario occur because of the server status update?
Right. "server-status.properties"
in a server was WRITABLE
but READ_ONLY
was applied from reply logs.
could a CD server exist without the dogma project?
No, impossible. However, a server's status mode can be changed after the server starts successfully. I don't think it will be a problem for new servers because the existing data in a replica should be copied to the new ones before starting them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to ask one more question, what do you think of allowing null
ids when creating mirrors and credentials from a usability perspective. CD server could generate random ids when no id is specified.
ID is required to update the existing data. Previously, users updated the whole data such as |
I missed this point. A mirror configuration can refer to a credential so After I think about it in many ways, I still think users have to create and manage their own IDs. |
Sure, I'm not sure what the UI will look like. If there is a good way for users to know in advance the used ids, I guess it will be fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the changes will be released together with the new mirroring UI. Looks good to me once the build passes 👍 👍 👍
This PR is going to be released before the mirroring UI. It may be difficult to detect bugs and handle them if this change was released along with the mirroring UI, so I wanted to release it separately. Do you have any concerns about that? |
I assume this comment is for the record since we already talked about this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically looks great. 👍 👍 👍
common/src/main/java/com/linecorp/centraldogma/internal/api/v1/MirrorDto.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMirroringService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/storage/repository/MetaRepository.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/internal/api/MirroringServiceV1.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/internal/api/MirroringServiceV1.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java
Outdated
Show resolved
Hide resolved
...java/com/linecorp/centraldogma/server/internal/storage/repository/DefaultMetaRepository.java
Show resolved
Hide resolved
@Post("/projects/{projectName}/credentials") | ||
@ConsumesJson | ||
@StatusCode(201) | ||
public CompletableFuture<PushResultDto> createCredential(@Param String projectName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So users always have to create the credential first before creating a mirror configuration.
Would it be worth if we provide an API that commits both together at once ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A credential can be used in multiple mirror configs so the relation is 1(credential):N(mirror).
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks excellent.
Thanks a lot, @ikhoon! 👍 👍 👍
if (GIT_MIRROR_ENABLED) { | ||
sb.annotatedService(API_V1_PATH_PREFIX, | ||
new MirroringServiceV1(projectApiManager, executor), decorator, | ||
v1RequestConverter, jacksonRequestConverterFunction, v1RequestConverter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: v1ResponseConverter
v1RequestConverter, jacksonRequestConverterFunction, v1RequestConverter); | ||
sb.annotatedService(API_V1_PATH_PREFIX, | ||
new CredentialServiceV1(projectApiManager, executor), decorator, | ||
v1RequestConverter, jacksonRequestConverterFunction, v1RequestConverter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v1ResponseConverter
// 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.)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add numMigratedProjects
to this log?
common/build.gradle
Outdated
@@ -7,6 +7,7 @@ dependencies { | |||
implementation libs.jackson.core | |||
implementation libs.jackson.databind | |||
implementation libs.jackson.datatype.jsr310 | |||
implementation libs.jackson.datatype.jdk8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to remove this line.
@ikhoon Could you check the failure? |
Motivation:
The ID for mirroring and credential configurations is optional, so I found it difficult to safely update a configuration with REST API. If a user updates a file manually on UI or commit API, the REST API may update a wrong configuration without a unique ID.
@trustin suggested changing the directory layout to store a configuration to a file with a unique ID. #838 (review)
This PR has three major changes:
.bak
suffix. e,g.mirrors.json
->mirrors.json.bak
,credentials.json
->credentials.json.bak
.Modifications:
MirroringMigrationService
that is executed when a server starts and scan all/mirrors.json
and/credentials.json
in the meta repo of projects and migrate them to the new format.mirror-<projectName>-<localRepo>-<shortWord>
credential-<projectName>-<shortWord>
short_wordlist.txt
is used as the word database.Mirror
and related classes to haveid
,enabled
as required fields.CredentailServiceV1
andMirrorServiceV1
to serve REST API for CRU.DefaultMetaRepository
.RepositoryUri
to represent a repository-specific URI such as a Git repository URL.MirrorDto
to serialize a mirroring configuration.Mirror
represents a mirroring task, soMirror
is not suitable for serialization.MirrorCredential
is used as is instead of creating a new DTO.Result: