Redis database backend #206

bshifter · 2024-12-06T13:29:24Z

This PR introduces a new database backend: Redis.

Redis was designed to be an in-memory data store (cache), not a fully featured relational database, but it also implements 2 types of persistence: RDB, which is more like a periodic snapshot, and AOF, which gives real durability similar to what real databases can provide.

Since the data OpenWEC stores in its database is not particularly huge and the structure of the data is relatively simple (no complex table joins, etc.), Redis can be a good candidate for very fast and scalable data storage.
We didn't measure the performance of OpenWEC with CockroachDB, so referring to performance is not completely fair, and I am sure that with proper configuration OpenWEC would reach the same scaling possibility and speed with PostgreSQL as with persistent Redis, but storing volatile data like Heartbeats/stats and frequently changing unstructured data like Bookmarks in Redis seemed an intuitive choice.

Our main "trigger" for this PR however was the simple fact that other commercial implementations of WEC rely on Redis as a Bookmark backend. Supporting Redis would make the transition to OpenWEC much easier for those who already operate their Redis clusters

This implementation is not yet complete, though it is functional, aside from the migration aspect. Consider this a first step - a foundation for further discussion. Redis operates quite differently from relational databases, which adds complexity to the implementation. I deliberately avoided altering the original trait definition, even though some adjustments for uniformity could have simplified the process.

The core idea is to store each item in Redis using a key structure like {type}/{subscription}/{machine}, ensuring unique entries. For example: HEARTBEAT:1231234124:my-computer. The associated data types can vary based on the specific information that needs to be stored.

To address the absence of SQL-like query features, these have been implemented on the client side (e.g., for stats). The implementation of the migration part might require further discussion. While structural changes in Redis are unnecessary, updating existing data across versions could be crucial.

vruello · 2024-12-06T16:14:23Z

Thanks for this huge pull request! 👍

I will take a look at it in the next few days.

vruello · 2024-12-06T16:41:33Z

Just a first quick note: You don't need to "translate" existing migrations. A migration needs to be created when we make a change that alters the database schema or its content. Its goal is to ensure that OpenWEC users can safely upgrade from one version to another without having to reset their database (which is a must for production use).

For a new backend, you may want to create an initial migration to initialize the db schema if the DBMS requires it. If not, just make sure that we will be able to write migrations in the future to alter the db content.

Required for serializing Subscription structs into Redis values using serde_json. This simplifies storing the structs as JSON since their fields are irrelevant in the current context. serde_json is used for its ease and efficiency.

import deadpool_redis

Since redis is not a relational database and does not support queries like sql, stats need to be calculated on client side. This enum is about to help filtering machines by different terms.

bshifter · 2024-12-07T13:02:49Z

Clippy warnings are fixed!

bshifter · 2024-12-07T17:49:35Z

CI env var for redis fixed (hopefully)

vruello

Thanks again for this nice pull request! Here is a bunch of comments and suggestions.

vruello · 2024-12-11T16:47:19Z

common/src/database/redisdomain.rs

Why did you choose to create another file? For simplicity, I would prefer to have one file per db backend.

vruello · 2024-12-11T16:49:40Z

common/src/database/redisdomain.rs

+    Any,
+}
+
+impl fmt::Display for RedisDomain {


You could probably replace this by a strum macro, such as https://docs.rs/strum/latest/strum/derive.IntoStaticStr.html.

vruello · 2024-12-11T16:50:33Z

common/src/database/redisdomain.rs

+    }
+}
+
+impl RedisDomain {


Could be replaced by strum.

vruello · 2024-12-11T16:53:03Z

common/src/database/schema/redis/mod.rs

You don't need to implement the already existing migrations. If the Redis backend does not need to initialize anything, then no migrations are needed for now. register_migrations should be empty.

vruello · 2024-12-12T07:09:08Z

common/src/database/redis.rs

+//       SOFTWARE.
+//
+//
+#![allow(unused_imports)]


Why do we need this? If imports are not used, they should be removed.

vruello · 2024-12-12T17:04:53Z

common/src/database/redis.rs

+        for key in keys {
+            let subscription_json: Option<String> = conn.get(&key).await.context("Failed to get subscription data")?;
+
+            if let Some(subscription_json) = subscription_json {


(De)Serializing the internal data model of OpenWEC in the database has caused problems in the past.

Using the internal structure directly looks simple at first glance. However, if a change is made in the structure, all tests will work, we won't see any problem in dev environments BUT, in a real environment, after upgrading OpenWEC, loading subscriptions will most likely fail due to deserialization errors. This approach requires a lot of care when changing internal structures (and could be considered technical debt).

I would suggest using an intermediate data model that is (de)serialized in the database after a conversion step. The advantage here is that changing an internal structure will cause a compilation error in the conversion, which would alert the developer (or a reviewer) that a migration is probably needed. This comes at the cost of some pain in the code and some copying of data (but we don't care about copying subscriptions)

vruello · 2024-12-12T17:10:40Z

common/src/database/redis.rs

+        identifier: &str,
+    ) -> Result<Option<SubscriptionData>> {
+        let mut conn = self.pool.get().await.context("Failed to get Redis connection")?;
+        let first_pass_key = format!("{}:{}:{}", RedisDomain::Subscription, identifier, RedisDomain::Any);


Would it be easier to retrieve all subscriptions and do the filtering ourselves? The number of subscriptions should be relatively small (<100), so I don't think we'll see any performance impact. Also, this API is only used by the cli so we don't really care about being very fast.

vruello · 2024-12-13T14:42:32Z

common/src/database/redis.rs

+        let mut conn = self.pool.get().await.context("Failed to get Redis connection")?;
+
+        let key_filter = format!("{}:{}:{}",RedisDomain::Subscription, subscription.uuid().to_string().to_uppercase(), RedisDomain::Any);
+        let keys = list_keys(&mut conn, &key_filter).await?;


I assume that this is required since subscriptions are indexed by uuid and name. I would suggest indexing them by uuid, and then only one SET would be required here.

vruello · 2024-12-13T14:44:02Z

common/src/database/redis.rs

+        let first_pass_key = format!("{}:{}:{}", RedisDomain::Subscription, uuid.to_uppercase(), RedisDomain::Any);
+        let second_pass_key = format!("{}:{}:{}", RedisDomain::Subscription, RedisDomain::Any, uuid);
+
+        let keys = list_keys_with_fallback(&mut conn, &first_pass_key, &second_pass_key).await?;


Same suggestion as elsewhere to index subscriptions by uuid (and not by name).

vruello · 2024-12-13T15:43:59Z

common/src/database/redis.rs

+        migration.up(&mut conn).await?;
+        let key = MIGRATION_TABLE_NAME;
+        let version = migration.version();
+        let added_count: i64 = conn.zadd(key, version, version).await.context(format!("Unable to add version: {}", version))?;


It would be better to use with_context instead of context since it does not just contain a static str.

bshifter · 2024-12-13T16:08:53Z

Thank you for the review and helpful comments! I’ll need a couple of days to apply the suggested changes.

bshifter force-pushed the redis-db branch 3 times, most recently from 2f69940 to 978d3f5 Compare December 6, 2024 15:40

bshifter force-pushed the redis-db branch from 978d3f5 to 4883973 Compare December 6, 2024 17:29

bshifter added 2 commits December 6, 2024 18:54

redis-db: add skeleton for redis database trait

91d96d2

import deadpool_redis

bshifter force-pushed the redis-db branch from 4883973 to d909eb2 Compare December 6, 2024 17:55

bshifter added 7 commits December 7, 2024 14:01

redis-db: add implementation for subscription

10492cd

redis-db: add implementation for bookmarks

a475ca4

redis-db: add MachineStatusFilter

5b006a2

Since redis is not a relational database and does not support queries like sql, stats need to be calculated on client side. This enum is about to help filtering machines by different terms.

redis-db: add implementation for heartbeats

c4d484a

redis-db: add implementation for stats

9e1dca6

redis-db: add settings/config for redis

51adbfd

WIP migrations

729c2b9

bshifter force-pushed the redis-db branch from d909eb2 to 425d689 Compare December 7, 2024 13:02

redis-db: Database trait test + tests

03abc83

bshifter force-pushed the redis-db branch from 425d689 to 03abc83 Compare December 7, 2024 17:48

vruello requested changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redis database backend #206

Redis database backend #206

bshifter commented Dec 6, 2024

vruello commented Dec 6, 2024

vruello commented Dec 6, 2024

bshifter commented Dec 7, 2024

bshifter commented Dec 7, 2024

vruello left a comment

vruello Dec 11, 2024

vruello Dec 11, 2024

vruello Dec 11, 2024

vruello Dec 11, 2024

vruello Dec 12, 2024

vruello Dec 12, 2024

vruello Dec 12, 2024

vruello Dec 13, 2024

vruello Dec 13, 2024

vruello Dec 13, 2024

bshifter commented Dec 13, 2024

Redis database backend #206

Are you sure you want to change the base?

Redis database backend #206

Conversation

bshifter commented Dec 6, 2024

vruello commented Dec 6, 2024

vruello commented Dec 6, 2024

bshifter commented Dec 7, 2024

bshifter commented Dec 7, 2024

vruello 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

bshifter commented Dec 13, 2024