Repository navigation
fix(redis): make the chart values apply, pin the image, and let the client follow a failover (CCP-5827) - #249
Open
NikhilMM89 wants to merge 8 commits into
Open
NikhilMM89 wants to merge 8 commits into
NikhilMM89 wants to merge 8 commits into
Conversation
Clients connected directly to the redis master Service, whose selector follows the StatefulSet ordinal rather than whichever pod Sentinel has promoted, so a failover could not be followed even with Sentinel running. buildRedisOptions now emits ioredis sentinel options when REDIS_SENTINELS is set and falls back to host/port when it is not, so behaviour is unchanged until Sentinel is actually deployed. webhook-trigger.service and notification-pubsub.service each built their own host/port object rather than calling buildRedisOptions, so fixing the shared helper alone would have left both pinned to the master. Both now route through it. Every `new Redis(` and the Bull queue in the repo now derive their options from that one function. The chart gains global.config.redisSentinels and redisMasterName. Both default to null, and the env vars render only when set, so the default output is unchanged. This is the client half of CCP-5827 and is inert on its own. It must ship BEFORE the Sentinel topology change, not with it: enabling Sentinel moves the bitnami chart to a single -node StatefulSet and removes the -master Service this code currently points at.
Update `CstarCacheStore` to pass Sentinel connection options (`sentinels`, `masterName`, `sentinelPassword`) through `createRedisClient` so it follows failover like other Redis clients. Add a unit test to ensure Sentinel settings are used instead of pinning to `redisHost`. In the backend deployment chart, inject `REDIS_SENTINEL_PASSWORD` from the Redis secret because Sentinel auth is enabled by default and lookups fail without it.
Import conflict only: main added COMPLETED_JOB_RETENTION to webhook-trigger while this branch added buildRedisOptions. Both are used; kept both.
… (CCP-5827) Folded in from #256 so the whole of CCP-5827 reviews on one branch. This is the ticket's second problem - "the replica count drifted from the chart". charts/redis/values*.yml nested everything under a top-level `redis:` key, but the pipeline deploys bitnami/redis DIRECTLY rather than as a subchart, so Helm ignored the block entirely and every environment ran pure Bitnami defaults: resourcesPreset nano (192Mi, not 1Gi) and a floating :latest tag. That is why the replica count drifted, why the earlier memory fix never took effect, and why redis kept crash-looping on "Can't handle RDB format version". It also explains the ticket's FIRST problem. sentinel.enabled was already set to `true` in these values - it just never applied. Turning it on now is not a flip: it switches the chart to the redis-node topology and removes the -redis-master Service the backend connects to. Left explicitly disabled and documented; actually running Sentinel is the remaining piece of CCP-5827 and is not in this branch. The image is pinned to sha256:33a5a129, what PROD resolves :latest to today (Redis 8.10.2). Forward, not back: PROD's appendonlydir was written by 8.10.2, so an older image would hit the very RDB failure the pin prevents. values-prod.yml carries PROD's own storage class and size (netapp-file-standard, 8Gi); volumeClaimTemplates is immutable and 8Gi -> 1Gi is a shrink Kubernetes refuses. Rendered all three environments against the live cluster - every PVC class and size matches what exists, so none attempts an immutable change.
NikhilMM89
changed the title
One conflict: backend/src/queue/redis-connection.spec.ts, add/add. CCP-6026 (#253, Redis Monitoring) created the same spec file on main to cover Bull metrics, while this branch created it to cover buildRedisOptions. Different functions in the same module, so both sets of tests were kept - 12 tests, all passing. Combined under main's structure, with the bull mock and the module import after it, because the createQueue tests need the mock hoisted. buildRedisOptions does not touch Bull, so the mock does not affect it. redis-connection.ts itself auto-merged and kept both features: the sentinel options from this branch and QUEUE_METRICS_MAX_DATA_POINTS / QUEUE_MAX_STALLED_COUNT from CCP-6026. charts/redis values are untouched by the merge - #253 did not go near them - so the pinned digest and the PROD storage override are intact. Test files failing in src/queue went from 3 to 4, which is main's own doing: origin/main fails the same 4 with the same 171 passing, on the pre-existing @nestjs/swagger module-resolution error.
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two of the three problems in CCP-5827. Folds in what was #256 so the whole ticket reviews on one branch.
The ticket's three problems
Why the chart values never applied
charts/redis/values*.ymlnested everything under a top-levelredis:key, but the pipeline deploysbitnami/redisdirectly, not as a subchart — so Helm ignored the block entirely. Every environment has been running pure Bitnami defaults:resourcesPreset: nano→ 192Mi, not the intended 1Gi:latest, a floating tagThat is the ticket's replica-count drift. It is also why the earlier memory fix never took effect, and why redis kept crash-looping on
Can't handle RDB format version— a floating tag let the on-disk RDB format move underneath the running server.It explains the first problem too.
sentinel.enabledwas alreadytruein these values — it just never applied. So "no Sentinel is running" was never a missing setting; it was this bug.Why Sentinel is still off
Turning it on is not a flip. It switches the Bitnami chart to the
redis-nodetopology, which removes the-redis-masterService the backend connects to. That needs to land together with the client change and a deliberate migration, so it is left explicitly disabled and documented in the values.Actually deploying Sentinel is the remaining piece of CCP-5827 and is not in this branch. Worth tracking separately so the ticket does not look closed when failover still is not working.
The client half
Clients connected directly to the redis master Service, whose selector follows the StatefulSet ordinal rather than whichever pod Sentinel promoted — so a failover could not be followed even with Sentinel running.
buildRedisOptionsnow emits ioredis sentinel options whenREDIS_SENTINELSis set and falls back to host/port when it is not, so behaviour is unchanged until Sentinel is actually deployed.webhook-trigger.serviceandnotification-pubsub.serviceeach built their own host/port object, so fixing the shared helper alone would have left both pinned to the master; both now route through it.Image pinned forward
Pinned to
sha256:33a5a129— what PROD resolves:latestto today, Redis 8.10.2. Forward rather than back matters: PROD'sappendonlydirwas written by 8.10.2, so an older image would hit the very RDB-format failure the pin prevents. TEST (8.6.3) and DEV move up to the same version.PROD storage
The base values target
netapp-block-standard/1Gi, which match DEV and TEST. PROD was created onnetapp-file-standard/8Gi.volumeClaimTemplatesis immutable and 8Gi→1Gi is a shrink Kubernetes refuses, so without an override the upgrade fails on storage before fixing anything.values-prod.ymlnow carries PROD's own class and size.Where each environment stands
:latestPROD is the only environment this changes materially.
Verification
Rendered all three environments against the live cluster — every PVC class and size matches what exists, so no environment attempts an immutable change:
Backend: 127 tests pass. Three test files fail on a pre-existing
@nestjs/swaggermodule-resolution error that fails identically onmain.Rollout
Deliberate changes to expect on deploy:
This rolls the redis master, so a brief queue interruption — worth doing TEST first and PROD in a low-traffic window.
Also not addressed here: PROD redis replicas are CPU-throttled ~33% of the time against a
150mlimit.Thanks for the PR!
Deployments, as required, will be available below:
Please create PRs in draft mode. Mark as ready to enable:
After merge, new images are deployed in: