nv-2054-implement-redis-cluster-service #3177

p-fernandez · 2023-04-11T13:17:51Z

What change does this PR introduce?

Adds two in memory cluster providers (Redis based configuration and Elasticache provider; both using Redis Cluster downstream) and connects it to the API, enabling Cache Service and Distributed Lock service to take advantage of a Redis Cluster instance for improving the performance.
It also adds the tests and the CI/CD changes needed for those to run.

Why was this change needed?

One of the performance improvements we are executing.

Other information (Screenshots)

Several environment variables need to be added and set depending on the different environments configuration.
For DEV environment we would like to set:
IN_MEMORY_CLUSTER_MODE_ENABLED=true
ELASTICACHE_CLUSTER_SERVICE_HOST=<ElastiCache_URL>
ELASTICACHE_CLUSTER_SERVICE_PORT=<ElastiCache_PORT>

For CI/CD and tests environments we would like to set:
IN_MEMORY_CLUSTER_MODE_ENABLED=true
REDIS_CLUSTER_SERVICE_HOST=localhost
REDIS_CLUSTER_SERVICE_PORTS=[7000,7001,7002,7003,7004,7005]

linear · 2023-04-11T13:17:54Z

NV-2054 Implement Redis cluster service

Definition of done:

Implements a Redis Cluster service and its connection with the current system
Gets integrated with Cache service and Distributed Lock service.
Adds a Redis Cluster provider
Adds a Elasticache Cluster provider as it will be the one enabled in DEV environment for operations.
Allows if Elasticache env variables set to use it as provider and if not fallback to the Redis Cluster provider.
Adds tests and passes them.
Add action in CI/CD to run a Redis Cluster environment for testing purposes.

p-fernandez · 2023-04-11T13:18:35Z

.github/actions/setup-redis-cluster/action.yml

@@ -0,0 +1,15 @@
+name: Setup Novu Redis Cluster


Action to create a Redis Cluster to be used by the CI/CD in the tests.

p-fernandez · 2023-04-11T13:18:50Z

.github/workflows/test-api.yml

@@ -32,6 +32,7 @@ jobs:
    steps:
      - uses: actions/checkout@v3
      - uses: ./.github/actions/setup-project
+      - uses: ./.github/actions/setup-redis-cluster


Precondition for the tests.

scopsy · 2023-04-11T13:20:37Z

apps/api/src/.example.env

+REDIS_CACHE_SERVICE_HOST=localhost
 REDIS_CACHE_SERVICE_PORT=6379


Would prefer if by default cache will be disabled locally, wdyt?

~~If we have that mentioned in the documentation I do agree.~~

It is:

Caching is turned off by default, but can easily be activated by setting the following environment variables:

scopsy · 2023-04-11T13:21:20Z

apps/api/src/app/shared/services/distributed-lock/distributed-lock.service.spec.ts

@@ -10,6 +10,8 @@ import { DistributedLockService } from './distributed-lock.service';

 const originalRedisCacheServiceHost = process.env.REDIS_CACHE_SERVICE_HOST;
 const originalRedisCacheServicePort = process.env.REDIS_CACHE_SERVICE_PORT;
+const originalRedisClusterServiceHost = process.env.REDIS_CLUSTER_SERVICE_HOST;
+const originalRedisClusterServicePorts = process.env.REDIS_CLUSTER_SERVICE_PORTS;


Should this be?

Suggested change

const originalRedisClusterServicePorts = process.env.REDIS_CLUSTER_SERVICE_PORTS;

const originalRedisClusterServicePorts = process.env.REDIS_CLUSTER_SERVICE_PORT;

No, the name is right as this is for the case where we provide the Redis Cluster with the ports nodes into an array. Nothing to do with the Elasticache cluster set up. This is the fallback.

scopsy · 2023-04-11T13:22:07Z

apps/api/src/app/shared/services/in-memory-provider/in-memory-provider.service.ts

+      });
+
+      inMemoryProviderClient.on('ready', () => {
+        Logger.warn('In-memory cluster ready', LOG_CONTEXT);


Suggested change

Logger.warn('In-memory cluster ready', LOG_CONTEXT);

Logger.log('In-memory cluster ready', LOG_CONTEXT);

I think better suited as it's a success one

I set it up higher as the readiness was being a problem because my infra problems. I can revert it back. 👍🏻

scopsy · 2023-04-11T13:22:23Z

apps/api/src/app/shared/services/in-memory-provider/in-memory-provider.service.ts

+    const { host, ttl } = getConfig();
+
+    if (!host) {
+      Logger.log('Missing host for in-memory cluster provider', LOG_CONTEXT);


Suggested change

Logger.log('Missing host for in-memory cluster provider', LOG_CONTEXT);

Logger.warn('Missing host for in-memory cluster provider', LOG_CONTEXT);

p-fernandez · 2023-04-11T13:20:56Z

apps/api/src/.env.test

@@ -22,6 +22,19 @@ REDIS_CACHE_KEEP_ALIVE=
 REDIS_CACHE_FAMILY=
 REDIS_CACHE_KEY_PREFIX=

+IN_MEMORY_CLUSTER_MODE_ENABLED=true


For testing we will use the Redis Cluster instance we are adding to the CI/CD. For running locally it would be needed to run one for tests to pass unless IN_MEMORY_CLUSTER_MODE_ENABLED is set to false and the tests for the InMemoryService are skipped.

p-fernandez · 2023-04-11T13:21:47Z

apps/api/src/app/shared/services/cache/cache.service.ts

-        const stream = client.scanStream({
-          match: pattern,
-        });
+        const stream = this.inMemoryProviderService.inMemoryScan(pattern);


@djabarovgeorge here is where I had to do this proxy to solve the lack of availability of this method in the Redis Cluster instances.

p-fernandez · 2023-04-11T13:22:26Z

apps/api/src/app/shared/services/in-memory-provider/elasticache-cluster-provider.ts

+const DEFAULT_TTL_SECONDS = 60 * 60 * 2;
+const DEFAULT_CONNECT_TIMEOUT = 50000;
+const DEFAULT_KEEP_ALIVE = 30000;
+const DEFAULT_FAMILY = 4;
+const DEFAULT_KEY_PREFIX = '';
+const TTL_VARIANT_PERCENTAGE = 0.1;


Carrying away the existing Redis instance configurations. Worth to be reviewed.

p-fernandez · 2023-04-11T13:23:07Z

apps/api/src/app/shared/services/in-memory-provider/elasticache-cluster-provider.ts

+    dnsLookup: (address, callback) => callback(null, address),
+    redisOptions: {
+      tls: {},
+    },
+    /*
+     *  Disabled in Prod as affects performance
+     */
+    showFriendlyErrorStack: process.env.NODE_ENV !== 'prod',
+    slotsRefreshTimeout: 2000,


Specific Elasticache configuration but showFriendlyErrorStack.

p-fernandez · 2023-04-11T13:26:00Z

apps/api/src/app/shared/services/in-memory-provider/in-memory-provider.service.ts

+    }
+  }
+
+  public getOptions(): RedisOptions | undefined {


Downstream the Redis Cluster can have Redis Options set up and configured just like a single instance, but they live in a different place.

p-fernandez · 2023-04-11T13:28:43Z

apps/api/src/app/shared/services/in-memory-provider/in-memory-provider.service.ts

+      },
+    };
+
+    return this.isElasticacheEnabled() ? clusterProviders.elasticache : clusterProviders.redis;


If Elasticache configuration is set in the environment we use that provider and if not we fall back to the Redis Cluster provider that needs automated setting in the CI/CD and in your local environment when running the tests. If the Cluster mode is enabled in the configuration through IN_MEMORY_CLUSTER_MODE_ENABLED.

p-fernandez · 2023-04-11T13:30:13Z

apps/api/src/config/env-validator.ts

+  REDIS_CLUSTER_SERVICE_HOST: str({
+    default: '',
+  }),
+  REDIS_CLUSTER_SERVICE_PORTS: str({
+    default: '',
+  }),


Not sure if we should validate the Elasticache variables as they are optional and depending on the environment deployed.

p-fernandez · 2023-04-11T13:30:51Z

libs/testing/src/user.session.ts

@@ -36,16 +36,18 @@ import { UserService } from './user.service';
 /**
 * TODO: move this to a reusable area
 */
+const connection = {
+  db: Number(process.env.REDIS_DB_INDEX || '1'),
+  port: Number(process.env.REDIS_PORT || 6379),


This and other hardcodes regarding the port need to be addressed in another PR.

scopsy · 2023-04-11T13:44:16Z

apps/api/src/.env.test

+REDIS_CLUSTER_SERVICE_HOST=localhost
+REDIS_CLUSTER_SERVICE_PORTS=[7000,7001,7002,7003,7004,7005]


@p-fernandez I think we should remove this from the .env.test, so that tests are running against single node on our machines, but in ci we can override those env variables in runtime

We can do that just by setting: IN_MEMORY_CLUSTER_MODE_ENABLED to false. Just want to highlight that in-memory-provider.service.spec.ts won't pass.

Sure, we can do that

p-fernandez · 2023-04-12T07:59:44Z

apps/api/src/app/shared/services/in-memory-provider/elasticache-cluster-provider.ts

  };

+  Logger.log(`Initializing Elasticache Cluster with ${instances?.length} instances`);


This is not going to count the real instances as the set up for Elasticache is different. Not sure if through the URL or any API call we would know the exact number.

p-fernandez · 2023-04-12T07:59:58Z

apps/api/src/app/shared/services/in-memory-provider/redis-cluster-provider.ts

@@ -86,6 +87,8 @@ export const getRedisCluster = (): Cluster | undefined => {
    slotsRefreshTimeout: 2000,
  };

+  Logger.log(`Initializing Redis Cluster Provider with ${instances?.length} instances`);


This is ok. 👍🏻

LetItRock · 2023-04-12T08:38:30Z

.github/workflows/reusable-api-e2e.yml

-      - name: Run a test
+      - name: Run E2E tests
+        env:
+          IN_MEMORY_CLUSTER_MODE_ENABLED: true
        run: |
          cd apps/api && pnpm test:e2e
          pnpm test


this line will run unit tests

Ooops forgot to remove it

feat(api): redis cluster service

939105e

p-fernandez requested review from davidsoderberg, LetItRock, ainouzgali, scopsy, BiswaViraj, djabarovgeorge and Cliftonz April 11, 2023 13:17

p-fernandez self-assigned this Apr 11, 2023

github-actions bot added @novu/api @novu/ws labels Apr 11, 2023

p-fernandez commented Apr 11, 2023

View reviewed changes

p-fernandez force-pushed the nv-2054-implement-redis-cluster-service branch from 858b51a to cc53e0b Compare April 11, 2023 13:19

scopsy approved these changes Apr 11, 2023

View reviewed changes

feat(cluster): add elasticache provider and configs

6cbb287

p-fernandez force-pushed the nv-2054-implement-redis-cluster-service branch from cc53e0b to 6cbb287 Compare April 11, 2023 13:28

p-fernandez commented Apr 11, 2023

View reviewed changes

feat(cluster): pr fixes

43c642e

p-fernandez force-pushed the nv-2054-implement-redis-cluster-service branch from 3cf76e3 to 43c642e Compare April 11, 2023 13:38

scopsy reviewed Apr 11, 2023

View reviewed changes

scopsy added 2 commits April 11, 2023 16:47

fix: allow deploying dev override

3a756bb

fix: setup redis cluster in e2e

6546234

scopsy temporarily deployed to Development April 11, 2023 13:49 — with GitHub Actions Inactive

p-fernandez and others added 3 commits April 11, 2023 15:00

fix(cluster): disable cluster by default in test env

ba6c152

fix: enable cluster testing for e2e

7e6da79

fix(cluster): disable offline queue to avoid errors in startup

e12be5f

scopsy temporarily deployed to Development April 11, 2023 14:18 — with GitHub Actions Inactive

scopsy temporarily deployed to Development April 11, 2023 14:33 — with GitHub Actions Inactive

fix(cluster): config mods to remove errors

14c0a06

p-fernandez temporarily deployed to Development April 11, 2023 14:36 — with GitHub Actions Inactive

p-fernandez temporarily deployed to development April 11, 2023 14:50 — with GitHub Actions Inactive

p-fernandez and others added 2 commits April 11, 2023 17:24

fix(cluster): add node error listener

e01e89b

Merge branch 'next' into nv-2054-implement-redis-cluster-service

0141856

scopsy temporarily deployed to Development April 12, 2023 06:45 — with GitHub Actions Inactive

scopsy temporarily deployed to Development April 12, 2023 06:59 — with GitHub Actions Inactive

scopsy temporarily deployed to Development April 12, 2023 07:07 — with GitHub Actions Inactive

scopsy temporarily deployed to Development April 12, 2023 07:19 — with GitHub Actions Inactive

scopsy added 3 commits April 12, 2023 10:42

fix: config test

00fa6fa

fix: add cache service to health check

0d3ed8b

feat: add elasticache health indicator

5f6547d

scopsy temporarily deployed to Development April 12, 2023 07:58 — with GitHub Actions Inactive

p-fernandez commented Apr 12, 2023

View reviewed changes

scopsy added 2 commits April 12, 2023 11:01

fix: update log

55ac596

fix: restore required test_api on ci

31e6053

scopsy approved these changes Apr 12, 2023

View reviewed changes

scopsy temporarily deployed to Development April 12, 2023 08:28 — with GitHub Actions Inactive

LetItRock reviewed Apr 12, 2023

View reviewed changes

scopsy temporarily deployed to development April 12, 2023 08:43 — with GitHub Actions Inactive

fix: duplicate test run

e9ea174

scopsy merged commit c951d4b into next Apr 12, 2023
11 checks passed

scopsy deleted the nv-2054-implement-redis-cluster-service branch April 12, 2023 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nv-2054-implement-redis-cluster-service #3177

nv-2054-implement-redis-cluster-service #3177

p-fernandez commented Apr 11, 2023

linear bot commented Apr 11, 2023

p-fernandez Apr 11, 2023

p-fernandez Apr 11, 2023

scopsy Apr 11, 2023

p-fernandez Apr 11, 2023 •

edited

scopsy Apr 11, 2023

p-fernandez Apr 11, 2023

scopsy Apr 11, 2023

p-fernandez Apr 11, 2023

scopsy Apr 11, 2023

p-fernandez Apr 11, 2023

p-fernandez Apr 11, 2023

p-fernandez Apr 11, 2023

p-fernandez Apr 11, 2023

p-fernandez Apr 11, 2023

p-fernandez Apr 11, 2023

p-fernandez Apr 11, 2023

p-fernandez Apr 11, 2023

scopsy Apr 11, 2023

p-fernandez Apr 11, 2023

scopsy Apr 11, 2023

p-fernandez Apr 12, 2023

p-fernandez Apr 12, 2023

LetItRock Apr 12, 2023 •

edited

scopsy Apr 12, 2023

		REDIS_CACHE_SERVICE_HOST=localhost
		REDIS_CACHE_SERVICE_PORT=6379

	const originalRedisClusterServicePorts = process.env.REDIS_CLUSTER_SERVICE_PORTS;
	const originalRedisClusterServicePorts = process.env.REDIS_CLUSTER_SERVICE_PORT;

	Logger.warn('In-memory cluster ready', LOG_CONTEXT);
	Logger.log('In-memory cluster ready', LOG_CONTEXT);

	Logger.log('Missing host for in-memory cluster provider', LOG_CONTEXT);
	Logger.warn('Missing host for in-memory cluster provider', LOG_CONTEXT);

		REDIS_CLUSTER_SERVICE_HOST=localhost
		REDIS_CLUSTER_SERVICE_PORTS=[7000,7001,7002,7003,7004,7005]

		};

		Logger.log(`Initializing Elasticache Cluster with ${instances?.length} instances`);

nv-2054-implement-redis-cluster-service #3177

nv-2054-implement-redis-cluster-service #3177

Conversation

p-fernandez commented Apr 11, 2023

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-fernandez Apr 11, 2023 • edited

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

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

LetItRock Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-fernandez Apr 11, 2023 •

edited

LetItRock Apr 12, 2023 •

edited