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

Provide a way to run a Plugin in a zone leader replica #1044

Merged
merged 8 commits into from
Nov 7, 2024

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Oct 21, 2024

Motivation:

When Central Dogma works on multiple IDC, we may need to run a Plugin in a specific zone due to network latency, firewall policy, or load balancing.

Modifications:

  • Add PluginTarget.ZONE_LEADER_ONLY to run a Plugin in each zone leader.
  • Add ZoneProvider interface to dynamically resolve a zone name.
  • Allow setting zone via dogma.json and CentralDogmaBuilder.
    • If the value of zone starts with classpath:, the class is loaded and cast to ZoneProvider.
    • If the value starts with $, the value is read from the environment variable.
  • Add onTakeZoneLeadership and onReleaseZoneLeadership to the methods and fields where onTakeLeadership and onReleaseLeadership were defined before.
    • /leader/{zone} is used to take the zone leadership.

Result:

You can now run a Plugin in a zone leader with the following:

  • dogma.json
    {
       "zone": "my-zone" 
    }
  • Plugin
    class ZoneLeaderPlugin implements Plugin {
    
      @Override
      public PluginTarget target() {
        return PluginTarget.ZONE_LEADER_ONLY;
      }
    }

@ikhoon ikhoon added this to the 0.71.0 milestone Oct 21, 2024
@ikhoon ikhoon marked this pull request as ready for review October 22, 2024 06:28
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.

Basically looks great! Left a few suggestions. 😉

@@ -64,7 +64,8 @@ defaults:
"port": 36463,
"protocol": null,
"path": null
}
},
"zone": null
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be in replication because zone is only used when replication is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I designed PluginTarget.ZONE_LEADER_ONLY to run with a standalone mode for the convenience of testing.


import com.linecorp.armeria.common.util.SystemInfo;

final class ZoneResolver {
Copy link
Member

Choose a reason for hiding this comment

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

Because this converts a value in the configuration file, how about using ConfigValueConverter?
I also had the plan to add classpath support in DefaultConfigValueConverter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

classpath seems useful. That said, I decided not to add classpath support but use a custom ConfigValueConverter to dynamically load a zone name.
It would take some time for me to design whether to call the function by convention such as one method with no-args or provide an API. Let me revisit it when I come to a conclusion.

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Left a review on naming, but looks good 👍

@@ -262,6 +263,16 @@ Core properties

- the path of the management service. If not specified, the management service is mounted at ``/internal/management``.

- ``zone`` (string)
Copy link
Contributor

Choose a reason for hiding this comment

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

I do think zone may be confusing for users since it seems like a generic field which handles multi-zone deployment, but only affects plugin behavior - especially since there hasn't been a notion of zone in central dogma before (only internally).

What do you think of naming this more specifically if you don't have plans to expand the functionality of this field?

e.g.
plugin-group-id/plugin-zone-id

Alternatively, I think just using groupId of the server itself may be clearer - I think it is possible to deduce it from the peerId once a zk server starts.

Copy link
Contributor Author

@ikhoon ikhoon Nov 5, 2024

Choose a reason for hiding this comment

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

That's a good point. As you said, it's currently only used for plugins, but I didn't think the usage of it is limited to the plugins. It would also be useful to expose zone in the UI. When creating a mirroring job, I plan to take a zone to run. Textual information is better than groupId which is a number.

Additionally, we may allow creating repositories only in certain zones later, as cloud providers do.

Copy link
Contributor

Choose a reason for hiding this comment

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

By adding a coupling with zone, users won't be able to run plugins with arbitrary groups: i.e. run pluginA for ZoneA,ZoneB, run pluginB for ZoneC.

Having said this, I agree it's probably fine to proceed as-is since we haven't had requirements for this kind of use-case yet and we can always add a new Plugin type.

Textual information is better than groupId which is a number.

Sounds fine, but when developing this field further I think it may still be useful if the relationship between groupId and zone is better defined. (Perhaps we can maintain an internal mapping of zone <-> groupId if only numbers are allowed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds fine, but when developing this field further I think it may still be useful if the relationship between groupId and zone is better defined. (Perhaps we can maintain an internal mapping of zone <-> groupId if only numbers are allowed)

I will think about this part further while working on mirroring. I need to change the configuration format to take all available zones to expose as a select box in the following PR.
For example:

zone: {
  currentZone: "ZoneA",
  allZones: ["ZoneA", "ZoneB", "ZoneC"]
}

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.

Looks great! 👍 👍 👍

@ikhoon ikhoon merged commit 8ca35bf into line:main Nov 7, 2024
9 of 10 checks passed
@ikhoon ikhoon deleted the zone-leader-plugin branch November 7, 2024 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants