Adding DynamoDB high availability features #675

mcowgill-stripe · 2024-05-23T23:18:02Z

Context

The main purpose of this change is to enable the ability for external consumers to leverage high availability features without needing ZooKeeper and to be able to do that with configuration instead of needing to modify source code or re-implement logic that is available here today.

The first part of this is creating an extension to use DynamoDB for leader election and leader monitoring. We had submitted prior work from @crioux-stripe and myself to add a KV Store for DynamoDB. That started to divide the extension for a "store" project. Looking at what we'd have to add to separate the store from the leader election/monitoring it makes more sense to create an extension for just DynamoDB and include the store, leader election, and leader monitoring. In theory the only difference is the size of the dependencies and any of the three bring with it roughly the same size. However, if the maintainers have different opinions I'm happy to pivot.

Looking at the code today and projects/docs. The idea seems to be that mantis-control-plane-core is a common dependency between the server and client. This does have exceptions, but seems like it's the intent. With that in mind I designed to have the DynamoDB extension only takea dependency on core and I deprecated and moved things into core to enable that. If that's not the goal/intent of the design today, I'm happy to move all that back.

In order to finish this goal of providing a way to inject dependencies for DynamoDB there are additional changes that need to happen in MasterMain.java for mantis server and HighAvailabilityServices inside of the client configurations. The latter one is more blocking for our internal use case today, since it's harder to unwind the different client paths than it is to re-implement MasterMain.

In order to make sure I don't go down paths that the maintainers won't accept, I'm opening the PR with the initial DynamoDB code for leader election and leader monitoring. The minimal deprecations and refactoring needed to manage the dependency flow. I am only commenting near the code blocks to add injection. Should I continue in this PR for those sections or should I try to finish and merge this and return to refactor the design for those two code paths (MasterMain and HighAvailabilityServicesUtil).

Checklist

./gradlew build compiles code correctly
Added new tests where applicable
./gradlew test passes all tests
Extended README or added javadocs where applicable

...trol-plane-store-dynamodb/src/test/java/io/mantisrx/server/master/store/DynamoStoreTest.java

Andyz26 · 2024-05-24T17:28:29Z

@mcowgill-stripe overll lgtm. I see you still have some todo/comments so ping me when you think this is ready for merge.

github-actions · 2024-05-24T18:11:56Z

Test Results

533 tests +13 527 ✅ +13 7m 52s ⏱️ -15s
139 suites + 4 6 💤 ± 0
139 files + 4 0 ❌ ± 0

Results for commit 8744c4e. ± Comparison against base commit d773089.

This pull request removes 1 and adds 14 tests. Note that renamed tests count towards both.

io.mantisrx.server.master.store.KeyValueStoreTest ‑ testUpsertOrdered

io.mantisrx.extensions.dynamodb.DynamoDBClientSingletonTest ‑ highAvailabilityServices
io.mantisrx.extensions.dynamodb.DynamoDBLeaderElectorTest ‑ becomesCurrentLeader
io.mantisrx.extensions.dynamodb.DynamoDBLeaderElectorTest ‑ respectsExistingLock
io.mantisrx.extensions.dynamodb.DynamoDBLeaderElectorTest ‑ restartsOnFailures
io.mantisrx.extensions.dynamodb.DynamoDBLeaderElectorTest ‑ shutdownAsLeader
io.mantisrx.extensions.dynamodb.DynamoDBLeaderElectorTest ‑ shutdownNotAsLeader
io.mantisrx.extensions.dynamodb.DynamoDBMasterMonitorTest ‑ getCurrentLeader
io.mantisrx.extensions.dynamodb.DynamoDBMasterMonitorTest ‑ runShutdown
io.mantisrx.extensions.dynamodb.DynamoDBStoreTest ‑ testInsertAndDelete
io.mantisrx.extensions.dynamodb.DynamoDBStoreTest ‑ testInsertAndDeleteMoreThan25
…

♻️ This comment has been updated with latest results.

mcowgill-stripe · 2024-05-24T19:22:35Z

@Andyz26 - I've broken something in container tests it appears. It's not obvious to me what it is yet. I am trying to slowly walk things backwards until I can make this test pass again. If you have a suggestion that would probably accelerate me. If not I'll keep trying to bisect the change.

Regarding the TODO's this PR had reached a size where I wanted to pause and check in. We could merge these changes and open a new PR to complete those TODOs (assuming I fix the test issue above). Or I could finish proposing the change to enable injection for the high availability services.

Do you have a preference on the continuing this PR or trying to reach a merge point and separating into a new PR?

…ce, unsure why at this time

mcowgill-stripe · 2024-05-24T23:15:36Z

I am not sure if I ran into a class erasure issue with the naming or not. After finding that this commit passed locally, I chose to rename the KeyValueStore to IKeyValueStore to reduce that possibility.

…a factory

Andyz26 · 2024-05-28T17:13:22Z

@Andyz26 - I've broken something in container tests it appears. It's not obvious to me what it is yet. I am trying to slowly walk things backwards until I can make this test pass again. If you have a suggestion that would probably accelerate me. If not I'll keep trying to bisect the change.

Regarding the TODO's this PR had reached a size where I wanted to pause and check in. We could merge these changes and open a new PR to complete those TODOs (assuming I fix the test issue above). Or I could finish proposing the change to enable injection for the high availability services.

Do you have a preference on the continuing this PR or trying to reach a merge point and separating into a new PR?

IMO, as long as nothing is broken, it's up to you whether you prefer one PR or multiple.
For test errors, the test container can be debugged if you put a breakpoint or something and examine the error in your local Docker app, too.

…nfiguration

Andyz26 · 2024-05-29T18:28:51Z

...plane/mantis-control-plane-core/src/main/java/io/mantisrx/server/core/CoreConfiguration.java

@@ -78,4 +77,12 @@ public interface CoreConfiguration {
    @Config("mantis.asyncHttpClient.readTimeoutMs")
    @Default("10000")
    int getAsyncHttpClientReadTimeoutMs();
+
+    @Config("mantis.leader.elector.factory")
+    @Default("io.mantisrx.server.core.master.LocalLeaderFactory")


A few issues I see here:

This leaderElector is only for control plane so it doesn't need to be in coreConfig (and all the workerConfig related classes) for worker/agents.

We are actually trying to deprecate/move away from config-injected objects. In this case if the electorFactory is only used in control-plane startup I assume it can be just a string in masterConfig directly.

Happy to remove this part, I noticed it today as well.

I'm not a huge fan of the DI with Skife today (and it's a little out of date). So I'm happy to hear that position.

What are the current thoughts on how we should think about extending Mantis then? I started with the view point I should only take a dependency on core, this way client or server could later bundle my jar. The only option I could find was the object-injection with Skife today. Should I be looking for another pattern here?

Potentially we could separate each component (@crioux-stripe and I started there with store). That felt pretty heavy weight for each store and we duplicated a lot of DynamoDB config, so this time I went with just an extension for DynamoDB in a gradle project. We could separate each feature into subprojects in the Gradle project and choose different dependencies for each subproject:

mantis-ext-dynamodb * common (common dynamodb config each subject depends on common) * server [subproject only depends on mantis-control-plane-server] (contains store and leader elector) * leader-monitor [subproject depends on mantis-control-plane-core] (contains only monitor)

The above feels pretty heavy weight, but perhaps more precise? We'd have to document this somewhere as well and it's sort of straightforward, but definitely not clear when you look at the current source code.

I started with the view point I should only take a dependency on core, this way client or server could later bundle my jar. The only option I could find was the object-injection with Skife today. Should I be looking for another pattern here?
Are you saying you don't take a dependency on mantis-control-plane-server, so you don't have access to the MasterConfiguration?

For skife injection, we don't have a full plan yet on how to replace it but we are trying to remove existing usage and avoid adding new ones. Reflection with config string is an ok workaround for now IMO, or have some builder/ctor pass the instance from your wrapper module.
I am open to a zoom call etc. too if you want to further discuss this.

Are you saying you don't take a dependency on mantis-control-plane-server, so you don't have access to the MasterConfiguration?

There is no issue with that access that I see because that is working in my branch today (unless you know something I don't)

mcowgill-stripe · 2024-06-04T23:05:18Z

...trol-plane-dynamodb/src/main/java/io/mantisrx/extensions/dynamodb/DynamoDBMasterMonitor.java

+import rx.subjects.BehaviorSubject;
+
+@Slf4j
+public class DynamoDBMasterMonitor extends BaseService implements MasterMonitor {


This design is a simple polling design to monitor the key in DynamoDB. After I finished this code, I know it can be vastly improved. I chose to not push further because there is potential to extract this. Leader monitoring could be completely separate from leader election. In the current implementation ZK and DynamoDB leader monitoring is very knowledgable about how leader is elected. If we change the design to the leader publishes when elected, and heartbeats regularly on a common messaging framework, then monitoring can be separated. This would push requirements to leader election, but I think we could actually push those to leadership manager. That refactor I thought should be a completely separate PR and this implementation should meet the current need.

mcowgill-stripe · 2024-06-04T23:05:42Z

...ntis-control-plane-dynamodb/src/main/java/io/mantisrx/extensions/dynamodb/DynamoDBStore.java

+import software.amazon.awssdk.services.dynamodb.model.*;
+
+@Slf4j
+public class DynamoDBStore implements IKeyValueStore {


rename and not modified.

mcowgill-stripe · 2024-06-04T23:06:13Z

mantis-control-plane/mantis-control-plane-dynamodb/src/main/resources/dynamodb.properties

@@ -0,0 +1,8 @@
+# mantis.ext.dynamodb.store.table=mantis-dynamodb-kv


This is an example file, but since the config framework has defaults I left them commented out.

mcowgill-stripe · 2024-06-04T23:06:45Z

...lane-dynamodb/src/test/java/io/mantisrx/extensions/dynamodb/DynamoDBClientSingletonTest.java

+    }
+
+    /**
+     * The use of global state here and the fact the helper rules all start and stop servers around


Do we want to yank this out or is my trade off a good choice?

mcowgill-stripe · 2024-06-04T23:09:26Z

...-control-plane-dynamodb/src/test/java/io/mantisrx/extensions/dynamodb/DynamoDBStoreTest.java

+    public static final String V1 = "value1";
+    public static final String V4 = "value4";
+
+    @ClassRule public static DynamoDBLocalRule dynamoDb = new DynamoDBLocalRule();


Moved with only minor modifications to fit the Rule patterns.

mcowgill-stripe · 2024-06-04T23:12:25Z

.../mantis-control-plane-server/src/main/java/io/mantisrx/master/zk/ZookeeperLeaderElector.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZookeeperLeaderElector extends BaseService {


The original purpose was name change only, but that did grow as the CuratorService point of view required modifications to the constructor. I removed the builder model as I felt the factory eliminated the need for it. I did not see wide usage of that pattern and I left LeaderElector if users still want it.

mcowgill-stripe · 2024-06-04T23:13:11Z

...tis-control-plane-server/src/main/java/io/mantisrx/master/zk/ZookeeperLeadershipFactory.java

+import io.mantisrx.server.master.config.MasterConfiguration;
+import io.mantisrx.shaded.org.apache.curator.utils.ZKPaths;
+
+public class ZookeeperLeadershipFactory extends ZookeeperLeaderMonitorFactory implements ILeaderElectorFactory {


This felt awkward to write, but was required since the interface is split and core needed a monitor factory.

mcowgill-stripe · 2024-06-04T23:14:12Z