Provide a way to pin a mirror to a zone #1062

ikhoon · 2024-11-18T10:20:01Z

Motivation:

I propose running the mirror in a zone close to the git server or only in zones that are allowed access.

Modifications:

Add zone as an optional property to mirror configurations.
Add GET /api/v1/mirror/config API to provide zone-related configurations.
- The API is used to select a pinned zone on the mirror form.
Add zonePinned flag to MirroringServicePluginConfig to enable zone-pinned mirroring.
- The option is disabled by default.
- DefaultMirroringServicePlugin.target() returns PluginTarget.ZONE_LEADER_ONLY if zonePinned == true
- Fixed MirrorSchedulingService to only run a pinned zone if zonePinned == true
Add ZoneConfig to replace zone: string configuration.
- allZones is newly added to specify the list of zone names.
Breaking) Make Plugin.target() take CentralDogmaConfig as an argument.

Result:

You can now specify a zone where you want to perform mirroring.

Motivation: We propse to run mirror in a zone close to the git server or only in zones that are allowed access. Modifications: - Add `zone` as an optional property to mirror configurations. - Add `GET /api/v1/mirror/config` API to provide zone related configurations. - The API is used to select a pinned zone on the mirror form. - Add `zonePinned` flag to `MirroringServicePluginConfig` to enable zone-pinned mirroring. - The option is disabled by default. - `DefaultMirroringServicePlugin.target()` returns `PluginTarget.ZONE_LEADER_ONLY` if `zonePinned == true` - Fixed `MirrorSchedulingService` to only run a pinned zone if `zonePinned == true` - Add `ZoneConfig` to replace `zone: string` configuration. - `allZones` is newly added to specify the list of zone names. - Breaking) Make `Plugin.target()` take `CentralDogmaConfig` as an argument. Result: You can now specify a zone where you want to perform mirroring.

jrhee17

Left only nits but looks good to me 👍 👍

site/src/sphinx/setup-configuration.rst

jrhee17 · 2024-11-22T01:02:16Z

xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.java

-                          .add("configType", configType())
-                          .add("target", target())
+                          .add("configType", configType().getName())
+                          .add("target", PluginTarget.LEADER_ONLY)


Question) doesn't this plugin target all replicas?

Suggested change

.add("target", PluginTarget.LEADER_ONLY)

.add("target", target())

Oops... 😱

jrhee17 · 2024-11-22T01:17:11Z

...ain/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMirroringServicePlugin.java

-                          .add("target", target())
+                          .omitNullValues()
+                          .add("configType", configType().getName())
+                          .add("target", pluginTarget)


nit; target is null unless #target(CentralDogmaConfig) is called. Not sure if this is intentional or not.

It was intentional and I thought it was safe because target(CentralDogmaConfig) is called right after the plugin is initialized. pluginTarget may be not null for most of the object's lifecycle.

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

jrhee17 · 2024-11-22T01:44:58Z

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

+                              if (zoneConfig != null) {
+                                  String pinnedZone = m.zone();
+                                  if (pinnedZone == null) {
+                                      // Use the first zone if the mirror does not specify a zone.


Noted that the order of allZones matters

jrhee17 · 2024-11-22T02:07:57Z

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

+                                      pinnedZone = zoneConfig.allZones().get(0);
+                                  }
+                                  if (!pinnedZone.equals(currentZone)) {
+                                      // Skip the mirror if the zone does not match.


Question) Would it be worth checking if the pinnedZone is contained in ZoneConfig#allZones to detect ill-configured mirrors?

Most users may choose the pinnedZone from the UI's select box, but it also makes sense to validate on the server side.

Sorry, let me rephrase my concern.

For instance, if servers are deployed in JP1, JP2, JP3 and we decide to migrate (JP3 -> JP4), is there no need for us to be aware of failing mirror jobs? I think moving servers across zones is not such a rare occasion.

It makes more sense. Let me report a failure to MirrorListener.

Note that I also want to generalize the mirror listener so that it also reports when a k8s aggregator fails.

Are you planning to design a general listener to receive arbitrary events?

It sounds nice. I'm looking forward to the Central Dogma event bus. 🚌

jrhee17 · 2024-11-22T02:14:32Z

server/src/main/java/com/linecorp/centraldogma/server/PluginGroup.java

-    @Nullable
-    static PluginGroup loadPlugins(ClassLoader classLoader, PluginTarget target, CentralDogmaConfig config,
-                                   List<Plugin> plugins) {
+    static Map<PluginTarget, PluginGroup> loadPlugins(ClassLoader classLoader, CentralDogmaConfig config,


Note: Understood that with this change, we don't allow the same plugin to be added with different PluginTargets (i.e. PluginA+LEADER_ONLY, PluginA+ZONE_LEADER_ONLY)

Co-authored-by: jrhee17 <guins_j@guins.org>

… into zone-aware-mirroring

minwoox

Left some nits. Looks great. 👍

minwoox · 2024-11-22T10:02:06Z

server/src/main/java/com/linecorp/centraldogma/server/PluginGroup.java

+        pluginGroups.forEach((target, group) -> {
+            logger.debug("Loaded plugins for target {}: {}", target,
+                         group.plugins().stream().map(plugin -> plugin.getClass().getName())
+                              .collect(Collectors.toList()));


nit: toImmutableList()

minwoox · 2024-11-22T10:09:49Z

server/src/main/java/com/linecorp/centraldogma/server/ZoneConfig.java

+     * Creates a new instance.
+     */
+    @JsonCreator
+    public ZoneConfig(@JsonProperty("currentZone") String currentZone,


Probably better to remove Zone suffix?

{ ... "zone": { "current": "foo", "all": ["foo", "bar"] } }

It's possible, but I don't think all conveys the exact meaning, so I'd like to keep the current expression.

minwoox · 2024-11-22T10:47:37Z

...ain/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMirroringServicePlugin.java

+                if (mirroringServicePluginConfig.zonePinned()) {
+                    zoneConfig = cfg.zone();
+                    if (zoneConfig == null) {
+                        throw new IllegalStateException(


How about throwing this exception a little bit earlier? e.g. in target method.

zoneConfig won't be null because it was checked in

centraldogma/server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java

Lines 267 to 270 in 5f0a9a1

if (pluginsForZoneLeaderOnly != null) {

checkState(cfg.zone() != null,

"zone must be specified when zone leader plugins are enabled.");

}

I just added this logic for double checking.

Then, how about using assert? We use assert for this purpose. 😉

minwoox · 2024-11-22T11:26:58Z

server/src/main/java/com/linecorp/centraldogma/server/mirror/MirroringServicePluginConfig.java

    }

    /**
     * Returns the number of mirroring threads.
     */
-    @JsonProperty
+    @JsonProperty("numMirroringThreads")


It looks like a boilerplate but I prefer this pattern because it is more deterministic.
We also use a standard getter for boolean values for which is is dropped. I wasn't sure if that was intended or not.

centraldogma/server/src/main/java/com/linecorp/centraldogma/server/CentralDogmaConfig.java

Lines 500 to 503 in 5f0a9a1

@JsonProperty

public boolean isWebAppEnabled() {

return webAppEnabled;

}

Anyway, it is unrelated to this PR so I will revert it.

Yes, we're currently using it in a mixed manner, and it might be a good time to define a standard. 🤔

site/src/sphinx/mirroring.rst

minwoox

👍 👍 👍

minwoox · 2024-11-29T13:38:01Z

server/src/main/java/com/linecorp/centraldogma/server/ZoneConfig.java

+        if (!allZones.contains(currentZone)) {
+            throw new IllegalArgumentException("The current zone must be one of the all zones.");
+        }


nit:

Suggested change

if (!allZones.contains(currentZone)) {

throw new IllegalArgumentException("The current zone must be one of the all zones.");

}

checkArgument(allZones.contains(currentZone), "The current zone: %s, (expected: one of %s)",

currentZone, allZones);

I was lazy. I used the code generated by GitHub Copilot as is. 😅

minwoox

👍 👍 👍

ikhoon requested review from jrhee17, minwoox and trustin as code owners November 18, 2024 10:20

ikhoon added the new feature label Nov 18, 2024

ikhoon marked this pull request as draft November 18, 2024 10:20

ikhoon added this to the 0.71.0 milestone Nov 18, 2024

ikhoon added 2 commits November 19, 2024 10:44

fix test

511c429

lint

07ac385

ikhoon marked this pull request as ready for review November 19, 2024 02:17

ikhoon added 2 commits November 19, 2024 13:54

cleanup

278b022

lint

2613d46

jrhee17 approved these changes Nov 22, 2024

View reviewed changes

ikhoon and others added 3 commits November 22, 2024 11:19

Update site/src/sphinx/setup-configuration.rst

7bc2061

Co-authored-by: jrhee17 <guins_j@guins.org>

address comments by @jrhee17

dbc7489

Merge branch 'zone-aware-mirroring' of github.com:ikhoon/centraldogma…

c36dbbb

… into zone-aware-mirroring

minwoox reviewed Nov 22, 2024

View reviewed changes

address comments

59b37e1

minwoox approved these changes Nov 29, 2024

View reviewed changes

fix validation

e404455

minwoox modified the milestones: 0.71.0, 0.72.0 Dec 4, 2024

ikhoon merged commit 3a261ba into line:main Dec 5, 2024
10 checks passed

ikhoon deleted the zone-aware-mirroring branch December 5, 2024 08:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to pin a mirror to a zone #1062

Provide a way to pin a mirror to a zone #1062

ikhoon commented Nov 18, 2024 •

edited

Loading

jrhee17 left a comment

jrhee17 Nov 22, 2024

ikhoon Nov 22, 2024

jrhee17 Nov 22, 2024

ikhoon Nov 22, 2024

jrhee17 Nov 22, 2024

jrhee17 Nov 22, 2024

ikhoon Nov 22, 2024

jrhee17 Nov 22, 2024

ikhoon Nov 29, 2024

minwoox Nov 29, 2024 •

edited

Loading

ikhoon Nov 29, 2024

minwoox Nov 29, 2024

ikhoon Nov 29, 2024

jrhee17 Nov 22, 2024

minwoox left a comment

minwoox Nov 22, 2024

minwoox Nov 22, 2024

ikhoon Nov 29, 2024

minwoox Nov 22, 2024

ikhoon Nov 29, 2024

ikhoon Nov 29, 2024

minwoox Nov 29, 2024

ikhoon Nov 29, 2024

minwoox Nov 22, 2024

ikhoon Nov 29, 2024

minwoox Nov 29, 2024

minwoox left a comment

minwoox Nov 29, 2024

ikhoon Dec 2, 2024

minwoox left a comment

	.add("target", PluginTarget.LEADER_ONLY)
	.add("target", target())

	if (pluginsForZoneLeaderOnly != null) {
	checkState(cfg.zone() != null,
	"zone must be specified when zone leader plugins are enabled.");
	}

	@JsonProperty
	public boolean isWebAppEnabled() {
	return webAppEnabled;
	}

Provide a way to pin a mirror to a zone #1062

Provide a way to pin a mirror to a zone #1062

Conversation

ikhoon commented Nov 18, 2024 • edited Loading

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon commented Nov 18, 2024 •

edited

Loading

minwoox Nov 29, 2024 •

edited

Loading