Introduce new mirroring and credential settings format and REST API #880

ikhoon · 2023-09-06T13:17:36Z

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:

Migrate the old mirror and credential settings to the new layout.

- mirrors
  - <mirror-id>.json
  - ...
- credentials
  - <credentials-id>.json
  - ...

Add a migration job to automatically migrate the old settings to the new format when a server starts. The old files are renamed by adding .bak suffix. e,g. mirrors.json -> mirrors.json.bak, credentials.json -> credentials.json.bak.
Add REST API for mirroring and credential configurations. This is a necessary task to add mirror UI. Add Mirroring and Credential UI #838

Modifications:

Add 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.
- "id" is a required property in each configuration. Human-readable random words are used to create a unique ID.
  - Mirror ID format: mirror-<projectName>-<localRepo>-<shortWord>
  - Credential ID format: credential-<projectName>-<shortWord>
  - short_wordlist.txt is used as the word database.
Change Mirror and related classes to have id, enabled as required fields.
Add CredentailServiceV1 and MirrorServiceV1 to serve REST API for CRU.
- Create, read, and update operations are implemented in DefaultMetaRepository.
Add RepositoryUri to represent a repository-specific URI such as a Git repository URL.
Add MirrorDto to serialize a mirroring configuration. Mirror represents a mirroring task, so Mirror is not suitable for serialization.
- MirrorCredential is used as is instead of creating a new DTO.
Migrated all mirroring tests to use the new configuration format.
Updated site documentation with the new format.

Result:

Mirroring and credential settings have been updated to the new formats.
You can now access and modify mirroring and credential resources using the REST API.

TBU

codecov · 2023-09-08T07:20:42Z

Codecov Report

Attention: Patch coverage is 75.76792% with 142 lines in your changes are missing coverage. Please review.

Project coverage is 70.60%. Comparing base (aae194c) to head (4ce5319).
Report is 11 commits behind head on main.

❗ Current head 4ce5319 differs from pull request most recent head f8091ae. Consider uploading reports for the commit f8091ae to get more accurate results

Files	Patch %	Lines
...necorp/centraldogma/internal/api/v1/MirrorDto.java	46.77%	19 Missing and 14 partials ⚠️
...rnal/storage/repository/DefaultMetaRepository.java	73.55%	20 Missing and 12 partials ⚠️
...ver/internal/mirror/MirroringMigrationService.java	80.76%	22 Missing and 8 partials ⚠️
...ver/internal/storage/repository/RepositoryUri.java	39.13%	14 Missing ⚠️
...erver/internal/mirror/DefaultMirroringService.java	50.00%	10 Missing ⚠️
...er/storage/project/InternalProjectInitializer.java	72.72%	3 Missing and 3 partials ⚠️
...ldogma/server/internal/api/MirroringServiceV1.java	85.71%	4 Missing ⚠️
...com/linecorp/centraldogma/server/CentralDogma.java	91.66%	1 Missing and 2 partials ⚠️
...ecorp/centraldogma/server/CentralDogmaBuilder.java	0.00%	3 Missing ⚠️
...corp/centraldogma/server/mirror/MirrorContext.java	71.42%	2 Missing ⚠️
... and 5 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #880      +/-   ##
============================================
+ Coverage     66.86%   70.60%   +3.74%     
- Complexity     3529     3748     +219     
============================================
  Files           370      375       +5     
  Lines         14531    14917     +386     
  Branches       1563     1597      +34     
============================================
+ Hits           9716    10532     +816     
+ Misses         3936     3436     -500     
- Partials        879      949      +70

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

minwoox

Looks great all in all. 👍 👍 👍

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

...va/com/linecorp/centraldogma/server/internal/mirror/MirroringAndCredentialServiceV1Test.java

...java/com/linecorp/centraldogma/server/internal/storage/repository/DefaultMetaRepository.java

...rc/main/java/com/linecorp/centraldogma/server/internal/storage/repository/RepositoryUri.java

...a/com/linecorp/centraldogma/server/internal/mirror/credential/PublicKeyMirrorCredential.java

jrhee17

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

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

jrhee17 · 2023-09-25T09:51:26Z

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

+        final JsonNode credentialId = mirror.get("credentialId");
+        if (credentialId != null) {


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

server/src/main/java/com/linecorp/centraldogma/server/internal/api/MirroringServiceV1.java

Motivation: While mirroring migration job is implemeted line#880, read-only mode is necessary to safely migrate the legacy `mirrors.json` and `credentials.json` the HEAD revision. As the current read-only mode rejects all commands, the migration job is also unable to push commits. A new command type is required to rejects only users requests but allows administrative cases. I propose to introduce two new command types for the smooth migration job. Modifications: - Add `UpdateServerStatusCommand` so as to toggle the read-only mode for the cluster. - Add `ForcePush` so as to wrap a push `Command` to be executed even in the read-only mode. Result: `UpdateServerStatusCommand` and `ForcePush` have been added as new commands to update the cluster status and forcibly push commits in the read-only mode.

Motivation: While mirroring migration job is implemeted #880, read-only mode is necessary to safely migrate the legacy `mirrors.json` and `credentials.json` the HEAD revision. As the current read-only mode rejects all commands, the migration job is also unable to push commits. A new command type is required to rejects only users requests but allows administrative cases. I propose to introduce two new command types for the smooth migration job. Modifications: - Add `UpdateServerStatusCommand` so as to toggle the read-only mode for the cluster. - Add `ForcePush` so as to wrap a push `Command` to be executed even in the read-only mode. Result: `UpdateServerStatusCommand` and `ForcePush` have been added as new commands to update the cluster status and forcibly push commits in the read-only mode.

ikhoon · 2024-02-14T11:59:22Z

I found a bug in updating server's status during rolling restart. Let me send a separate PR to handle it first.

ikhoon · 2024-02-21T13:10:46Z

I will proceed with this PR after #921 is merged.

ikhoon added 5 commits September 6, 2023 15:00

Add Mirroring and Credential REST API

8b51e01

TBU

Merge branch 'main' into mirror-api

eabb4d1

fix compile errors

eb28015

clean up

785e8dc

Migrate the legacy single file format to directory based format

1de0159

ikhoon added the new feature label Sep 6, 2023

ikhoon added 2 commits September 8, 2023 01:15

Fix broken mirror and credential tests

9444f37

Fix the wrong package name and update documentation

62533e4

ikhoon changed the title ~~Add Mirroring and Credential REST API~~ Introduce new mirroring and credential settings format and REST API Sep 8, 2023

ikhoon added the breaking change label Sep 8, 2023

ikhoon added this to the 0.62.2 milestone Sep 8, 2023

ikhoon marked this pull request as ready for review September 8, 2023 07:54

ikhoon requested review from jrhee17, minwoox and trustin as code owners September 8, 2023 07:54

Remove redundant validation

3b644b3

minwoox reviewed Sep 11, 2023

View reviewed changes

Address comments by @minwoox

ad000b7

jrhee17 reviewed Sep 25, 2023

View reviewed changes

...rc/main/java/com/linecorp/centraldogma/server/internal/mirror/MirroringMigrationService.java Outdated Show resolved Hide resolved

--wip-- [skip ci]

2abe5c4

ikhoon removed this from the 0.63.0 milestone Oct 30, 2023

ikhoon marked this pull request as draft October 30, 2023 03:03

ikhoon added 6 commits November 15, 2023 16:55

Merge branch 'main' into mirror-api

ed4da7d

Use MIRROR_PROVIDERS to check if Git mirror is enabled

62c6851

Address all comments

2083cdb

lint

9ae68a0

clean up

a7cb15d

javadoc

bfefa1a

ikhoon added 2 commits November 16, 2023 22:19

Add TODO

157d3a6

Add integration tests with ZooKeeper cluster

280c82e

ikhoon commented Nov 28, 2023

View reviewed changes

server/src/main/java/com/linecorp/centraldogma/server/internal/api/MirroringServiceV1.java Outdated Show resolved Hide resolved

Add validation

52af015

ikhoon force-pushed the mirror-api branch from f994024 to 52af015 Compare November 28, 2023 07:25

ikhoon mentioned this pull request Nov 28, 2023

Add AdministrativeCommands #899

Merged

ikhoon added 5 commits February 14, 2024 13:08

Merge branch 'main' into mirror-api

92ecae9

Read backup files to check if mirror migration was performed

faf081e

Follow up after merging

4ce5319

Add rollback logic

a9213fb

fix compile error

f3da444

ikhoon marked this pull request as ready for review February 14, 2024 09:32

ikhoon added this to the 0.65.0 milestone Feb 14, 2024

ikhoon added 2 commits February 14, 2024 18:52

Fix flakyness

bc200ef

lint

3399bfb

ikhoon marked this pull request as draft February 14, 2024 11:56

minwoox modified the milestones: 0.65.0, 0.66.0 Apr 18, 2024

Merge branch 'main' into mirror-api

f8091ae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new mirroring and credential settings format and REST API #880

Introduce new mirroring and credential settings format and REST API #880

ikhoon commented Sep 6, 2023 •

edited

codecov bot commented Sep 8, 2023 •

edited

minwoox left a comment

jrhee17 left a comment

jrhee17 Sep 25, 2023

ikhoon commented Feb 14, 2024

ikhoon commented Feb 21, 2024

		final JsonNode credentialId = mirror.get("credentialId");
		if (credentialId != null) {

Introduce new mirroring and credential settings format and REST API #880

Are you sure you want to change the base?

Introduce new mirroring and credential settings format and REST API #880

Conversation

ikhoon commented Sep 6, 2023 • edited

codecov bot commented Sep 8, 2023 • edited

Codecov Report

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

jrhee17 Sep 25, 2023

Choose a reason for hiding this comment

ikhoon commented Feb 14, 2024

ikhoon commented Feb 21, 2024

ikhoon commented Sep 6, 2023 •

edited

codecov bot commented Sep 8, 2023 •

edited