Adding Lag-dependent implementation of Redis streams scaler #4592

mikelam-us · 2023-05-31T01:08:25Z

Provide a description of what has been changed

Checklist

When introducing a new scaler, I agree with the scaling governance policy
I have verified that my change is according to the deprecations & breaking changes policy
Tests have been added
Changelog has been updated and is aligned with our changelog requirements
A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
A PR is opened to update the documentation on (repo) (if applicable)
Commits are signed with Developer Certificate of Origin (DCO - learn more)

This is a PR for a Redis scaler implementation based optionally on the lag count of a given consumer group. Previously, the scaler operated based either on the number of pending entries (as returned by XPENDING) or the stream length (as returned by XLEN), neither of which accurately reflect how the job should actually be scaled. The lag-based implementation should remedy this since consumer group lag much more accurately reflects how the job should be scaled. The implementation is backwards compatible with all other parts of keda, though it should be noted that Redis 7.0.0+ is required as XINFO lag, the Redis command on which this implementation depends, is absent in earlier versions.

All lag-specific unit tests have been added to redis_scaler_test.go. End-to-end tests are in redis_sentinel_streams_lag_test.go and redis_cluster_streams_lag_test.go.

Related issues: #4277, #3127

pkg/scalers/redis_streams_scaler.go

krishnadurai

@mikelam-us can you please address these comments I've left on your PR?

config/manager/kustomization.yaml

config/metrics-server/kustomization.yaml

config/webhooks/kustomization.yaml

pkg/scalers/redis_streams_scaler.go

tests/scalers/redis/redis_sentinel_streams_lag/redis_sentinel_streams_lag_test.go

.../redis/redis_sentinel_streams_pending_entries/redis_sentinel_streams_pending_entries_test.go

...rs/redis/redis_cluster_streams_pending_entries/redis_cluster_streams_pending_entries_test.go

tests/scalers/redis/redis_sentinel_streams_lag/redis_sentinel_streams_lag_test.go

CHANGELOG.md

pkg/scalers/redis_streams_scaler.go

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

…renamed test files Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

zroubalik · 2023-06-06T19:19:58Z

/run-e2e redis
Update: You can check the progress here

JorTurFer

Looking good. I left one comment inline
BTW, the standalone e2e test case is still pending 🙏

tests/scalers/redis/redis_cluster_streams_lag/redis_cluster_streams_lag_test.go

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

mikelam-us · 2023-06-09T20:31:55Z

Hello all! I've addressed all the previous comments, so it would be great if you all could take another look just to make sure I'm not missing anything! Thanks in advance!

JorTurFer

Sorry for the insistence, but could you cover redis_standalone case with e2e test too (other cases are already covered for that scenario)?

Apart from that, just a small comment inline

tests/scalers/redis/redis_cluster_streams_lag/redis_cluster_streams_lag_test.go

tests/scalers/redis/redis_sentinel_streams_lag/redis_sentinel_streams_lag_test.go

JorTurFer · 2023-06-12T22:03:14Z

/e2e-test redis

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

mikelam-us · 2023-06-19T20:48:20Z

Jorge, thank you so much for your comments on the e2e tests. While we were working on implementing these tests, we caught some interesting behavior with Redis. The initial consumer Deployment isn’t able to register its consumerGroup because by the time it is ready or otherwise, the producer Job hasn’t created the stream as it is not deployed by then. Now, since there is no lagCount, the consumer Deployment is scaled down to 0. We need the XLEN for the stream in order for our system to start (scale-up from zero) the consumer Deployments. We are dependent on the XLEN for start-up.
On all other scenarios, we default to lagCount from the XINFO because after the start-up event, we can reliably get the lagCount from the registered consumer group of our consumer that was brought up once already by the start-up process.
We spend the past week implementing this fix, and this should be reflected in the current PR. It would be great if you could take a look! Thanks!

Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>

JorTurFer · 2023-06-20T10:20:33Z

/run-e2e redis
Update: You can check the progress here

JorTurFer

Looking good, only 1 small change in the key name and that's all here

pkg/scalers/redis_streams_scaler.go

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

JorTurFer

LGTM!
Thanks a lot for the feature ❤️

JorTurFer · 2023-06-20T16:34:04Z

/run-e2e redis
Update: You can check the progress here

zroubalik

LGTM

thanks for the contribution!

…#4592) Signed-off-by: mikelam-us <michael.lam@aixplain.com> Signed-off-by: mikelam-us <69995279+mikelam-us@users.noreply.github.com> Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es> Co-authored-by: mikelam-us <michael.lam@aixplain.com> Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>

mikelam-us requested a review from a team as a code owner May 31, 2023 01:08

mikelam-us marked this pull request as draft May 31, 2023 01:08

github-advanced-security bot found potential problems May 31, 2023

View reviewed changes

pkg/scalers/redis_streams_scaler.go Fixed Show fixed Hide fixed

krishnadurai mentioned this pull request Jun 1, 2023

Redis 7.0 Streams - use Lag instead of pending #3127

Closed

krishnadurai suggested changes Jun 1, 2023

View reviewed changes

krishnadurai reviewed Jun 1, 2023

View reviewed changes

tests/scalers/redis/redis_sentinel_streams_lag/redis_sentinel_streams_lag_test.go Outdated Show resolved Hide resolved

krishnadurai reviewed Jun 1, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

krishnadurai reviewed Jun 1, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/scalers/redis_streams_scaler.go Outdated Show resolved Hide resolved

mikelam-us-aixplain added 22 commits June 1, 2023 12:47

Running e2e tests on new scaler

94af8c4

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

First hopefully successful test

80ca2e7

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Scaler works now

0cb0cb8

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Pull request

d3210ff

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Cleaned up redis streams scaler to support xlag, xpending, and xlength

0686070

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Added tests and changed 'lag' to 'lagCount'

acba7ef

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Updated E2E tests to reflect lag -> lagCount

f33c76b

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Updated protoc version and DeleteKubernetes call

eeea1b8

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Added pending entries unit tests, corrected lagFactor error message, …

b6e1144

…renamed test files Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Removed files that no longer exist

fd45427

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Remove unneeded grpc files

824c17f

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Added unit tests and version check in scaler

0b8e64e

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Fixing extraneous files

bb4d922

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Unit tests working

bd97535

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Final saves

24bad85

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

int -> int64

f908ff6

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Fixed all int64 problems

ac738c2

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Moved test directories

32b0b49

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Use XINFO server to check version number

8f1c1ab

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Fixing style errors and docker repository

3db35a1

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Fixed indentation error

2809d18

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Still fixing indentation erro

bbee9eb

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

mikelam-us-aixplain added 6 commits June 1, 2023 15:07

More flexible Redis version check

a32a3ba

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Fixed version check

f01dc41

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Added activation value check andmade namespaces unique across tests

66c7e51

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Correcting stylistic errors

8b76e88

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Fixed unnecessary leading newline

4d27605

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Fixing formatting

6151763

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

JorTurFer reviewed Jun 6, 2023

View reviewed changes

tests/scalers/redis/redis_cluster_streams_lag/redis_cluster_streams_lag_test.go Outdated Show resolved Hide resolved

mikelam-us marked this pull request as ready for review June 7, 2023 16:30

Streamlined e2e test

e0e771e

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

mikelam-us marked this pull request as draft June 7, 2023 17:51

Problem pushing commits

0dcb105

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

mikelam-us marked this pull request as ready for review June 7, 2023 18:43

krishnadurai mentioned this pull request Jun 8, 2023

Added redis lag docs kedacore/keda-docs#1147

Merged

1 task

JorTurFer reviewed Jun 12, 2023

View reviewed changes

tests/scalers/redis/redis_cluster_streams_lag/redis_cluster_streams_lag_test.go Outdated Show resolved Hide resolved

tests/scalers/redis/redis_sentinel_streams_lag/redis_sentinel_streams_lag_test.go Outdated Show resolved Hide resolved

mikelam-us-aixplain added 2 commits June 19, 2023 10:02

Fixed redis activation value bug and added standalone tests

ec2ce46

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Updated in-line redis scaler documentation

d6f442c

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Merge branch 'main' into main

2120ecf

Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>

JorTurFer reviewed Jun 20, 2023

View reviewed changes

pkg/scalers/redis_streams_scaler.go Outdated Show resolved Hide resolved

mikelam-us-aixplain added 2 commits June 20, 2023 09:02

Changed activationTargetLag -> activationLagCount

801b55d

Signed-off-by: mikelam-us <michael.lam@aixplain.com>

Merge branch 'main' of https://github.com/mikelam-us/keda

d8b448a

JorTurFer approved these changes Jun 20, 2023

View reviewed changes

zroubalik approved these changes Jun 21, 2023

View reviewed changes

zroubalik merged commit 96fc64c into kedacore:main Jun 21, 2023
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Lag-dependent implementation of Redis streams scaler #4592

Adding Lag-dependent implementation of Redis streams scaler #4592

mikelam-us commented May 31, 2023 •

edited

krishnadurai left a comment

zroubalik commented Jun 6, 2023 •

edited by github-actions bot

JorTurFer left a comment

mikelam-us commented Jun 9, 2023

JorTurFer left a comment •

edited

JorTurFer commented Jun 12, 2023

mikelam-us commented Jun 19, 2023

JorTurFer commented Jun 20, 2023 •

edited by github-actions bot

JorTurFer left a comment

JorTurFer left a comment

JorTurFer commented Jun 20, 2023 •

edited by github-actions bot

zroubalik left a comment

Adding Lag-dependent implementation of Redis streams scaler #4592

Adding Lag-dependent implementation of Redis streams scaler #4592

Conversation

mikelam-us commented May 31, 2023 • edited

Checklist

krishnadurai left a comment

Choose a reason for hiding this comment

zroubalik commented Jun 6, 2023 • edited by github-actions bot

JorTurFer left a comment

Choose a reason for hiding this comment

mikelam-us commented Jun 9, 2023

JorTurFer left a comment • edited

Choose a reason for hiding this comment

JorTurFer commented Jun 12, 2023

mikelam-us commented Jun 19, 2023

JorTurFer commented Jun 20, 2023 • edited by github-actions bot

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer left a comment

Choose a reason for hiding this comment

JorTurFer commented Jun 20, 2023 • edited by github-actions bot

zroubalik left a comment

Choose a reason for hiding this comment

mikelam-us commented May 31, 2023 •

edited

zroubalik commented Jun 6, 2023 •

edited by github-actions bot

JorTurFer left a comment •

edited

JorTurFer commented Jun 20, 2023 •

edited by github-actions bot

JorTurFer commented Jun 20, 2023 •

edited by github-actions bot