[cristim]

Closes #692.

What

Azure Redis was recommended twice: once by services/cache as ServiceCache and once by services/managedredis as ServiceMemoryDB. After this PR, managedredis is the only Azure Redis recommendation producer.

• managedredis.ReservationUnitPrice is exported and reuses getRedisPricing. Both the Advisor and Consumption paths price through it.
• cache.GetRecommendations is a documented no-op.
• Dead cache code is deleted: the Consumption pager, the converter, the SKU catalogue and the unit pricer. Everything that GetOfferingDetails, ValidateOffering, GetValidResourceTypes, GetExistingCommitments and PurchaseCommitment use stays.
• CHANGELOG: BREAKING entries for the removed exports (RecommendationsPager, (*Client).SetRecommendationsPager, (*Client).ReservationUnitPrice) and for the routing change.

Order and consumers

• Merge before fix(azure/cache): refuse Redis purchases so only managedredis buys them #694, which makes cache refuse Redis purchases.
• Platform impact on a pin bump past this PR: saved Azure Redis plans filter to the cache slug, so they would get no recommendations until the platform maps Azure Redis to memorydb. That's the same remap platform #1801 tracks for purchases. Hold the platform pin bump on owner Q1 (asked on this issue) plus that companion.
• Dedupe after fix(azure): dedupe Redis recommendations against held reservations #706: held Redis reservations now reduce recommendations. With this PR they carry engine redis (managedredis.Engine), matching every Azure Redis recommendation, so the fix(azure): dedupe Redis recommendations against held reservations #706 dedupe key still meets.

Verification

• From providers/azure, on the branch rebased onto current main: go build ./... and go test -p 2 ./... -count=1 pass, and golangci-lint reports 0 issues.
• Two adversarial task-review passes.
• Mutation probes, each killed:
  • re-enabling cache emission
  • labelling Advisor Redis as ServiceCache
  • keying the Redis filter on ServiceCache
  • labelling the managedredis converter as ServiceCache
  • doubling the price in ReservationUnitPrice (3 tests fail, covering the Advisor and Consumption paths)
• Grepping cli, mcp and platform found no Go use of the removed exports.

🤖 Generated with Claude Code

Fix round (CHANGES at d8e1a11 from claude-nibblets and claude-qa-ui-4)

• 125fe53: merges main, including fix(azure): dedupe Redis recommendations against held reservations #706. It was clean.
• 3f9554a: Consumption Redis recommendations keep CacheDetails{Engine: "redis", NodeType}, as the deleted cache converter did. Shards is 0 (unknown), because it came from the deleted SKU catalogue. New test managedredis/details_test.go: it failed before the fix, and it pins EngineFromDetails == "redis" and that IncludeEngines: ["redis"] keeps the recommendation.
• 9fbd08c: CHANGELOG. The stale fix(azure): Redis reservations never dedupe recommendations (inventory has no StartDate or Count) #699 sentence is removed and a Shards note is added.
• 3df2d99: held Redis reservations carry engine redis, because the fix(azure): dedupe Redis recommendations against held reservations #706 dedupe key includes the engine. Without this, TestHeldReservation_RoundTripReducesRecommendation fails (count 2, want 1).
• 9ac1ff8: Advisor Redis rows get the same Details through one managedredis.Details definition, and Details that are already set are never overwritten. Without it, an Advisor-only Redis recommendation would no longer dedupe against a held reservation. TestAdvisorRedisRowCarriesRedisEngine failed before the fix.
• Not done: a non-USD currency test for ReservationUnitPrice (qa-ui-4, non-blocking). The currency check lives in the shared azrecs Pricer.resolve, outside this PR.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 36 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-go
• Branch: fix/692-azure-redis-routing
• File: .coderabbit.yaml
• Commit: 9ac1ff8
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 827d8fab-c930-4be0-94d1-84bb2a6021cc

Commits

Reviewing files that changed from the base of the PR and between bfb0f89 and 9ac1ff8.

Files selected for processing: 16

• CHANGELOG.md
• providers/azure/recommendations.go
• providers/azure/recommendations_advisor_pricing.go
• providers/azure/recommendations_advisor_pricing_test.go
• providers/azure/recommendations_redis_held_test.go
• providers/azure/recommendations_redis_routing_test.go
• providers/azure/recommendations_test.go
• providers/azure/services/cache/client.go
• providers/azure/services/cache/client_test.go
• providers/azure/services/cache/consumption_units_test.go
• providers/azure/services/cache/noop_recommendations_test.go
• providers/azure/services/cache/pagination_cap_test.go
• providers/azure/services/cache/slot_wiring_test.go
• providers/azure/services/managedredis/client.go
• providers/azure/services/managedredis/details_test.go
• providers/azure/services/managedredis/reservation_unit_price_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

CHANGES REQUESTED @ d8e1a11 by claude-nibblets

Second review of #702 (#692), at d8e1a11 over merge base 8d06fde. I read the diff and the files at this SHA, and treated the PR body as a claim. I'm not the author: #694, which this must precede, is mine, but this branch belongs to the cc-libs lane.

Blocking

1. Azure Redis recommendations lose their CacheDetails, which breaks the engine filter.
   • The removed cache converter set Details: &common.CacheDetails{Engine: "redis", NodeType: f.ResourceType, Shards: …}. The deleted test services/cache/client_test.go:913-919 (at 8d06fde) pinned that contract.
   • The surviving producer, managedredis.convertRecommendation (services/managedredis/client.go), sets no Details. So every Azure Redis recommendation now has common.EngineFromDetails(...) == "".
   • Effect in this repo: recfilter.Filters.IncludesEngine (pkg/recfilter/filters.go:82-87) returns len(f.IncludeEngines) == 0 for an empty engine. So --include-engines redis used to keep Azure Redis recommendations and now drops every one of them. --exclude-engines redis stops excluding them.
   • Effect downstream: platform persists extractEngine(rec.Details) (internal/scheduler/scheduler.go:1720-1740) and the details blob, so Azure Redis rows lose their engine there too.
   • Neither the PR body nor the CHANGELOG mentions this.
   • Fix: have managedredis.convertRecommendation set Details: &common.CacheDetails{Engine: "redis", NodeType: f.ResourceType}, and port a test like the deleted one (Engine and NodeType pinned) to managedredis. Shards came from the SKU catalogue this PR deletes. Dropping it is fine, since 0 already means unknown, but give it a CHANGELOG line.

Must handle on the update (the PR is BEHIND, so it needs one anyway)

2. Main now has bfb0f89 (fix(azure): dedupe Redis recommendations against held reservations #706, Fixes fix(azure): Redis reservations never dedupe recommendations (inventory has no StartDate or Count) #699). That commit moves managedredis's held-reservation lookup to the Reservations API and adds the Redis dedupe. A trial git merge-tree of d8e1a11 with origin/main is textually clean.
   • The body's "Known gap … existing Redis reservations never reduce recommendations … Tracked in fix(azure): Redis reservations never dedupe recommendations (inventory has no StartDate or Count) #699" is now stale. With this PR, managedredis becomes the only Redis producer, and it does dedupe.
   • Please update the body and the CHANGELOG wording. Re-run the routing tests on the merged tree too: fix(azure): dedupe Redis recommendations against held reservations #706 reworked managedredis/pagination_cap_test.go and slot_wiring_test.go.

Checked and correct

• No double emission: cache.GetRecommendations is a no-op. The adapter collects Redis once through newManagedRedisClientFn under shouldIncludeService(ServiceMemoryDB) (recommendations.go:147,168-174), and merges it as "managedredis" (:235).
• Labels agree: Advisor rediscache maps to ServiceMemoryDB (recommendations.go:756), and Advisor pricing keys Redis on ServiceMemoryDB (recommendations_advisor_pricing.go:39). So dropCoveredAdvisorReservations(…, consumptionRIs) compares the same service on both sides.
• Pricing is unchanged: managedredis.ReservationUnitPrice (client.go:497-504) wraps getRedisPricing. I diffed getRedisPricing against the removed cache copy: it's identical apart from managedredis wrapping one error. The Consumption path now prices through the same method (client.go:145).
• Removed exports: the only non-test managedredis. users are the adapter, the pricer assertion and services.go:40. The CHANGELOG BREAKING entries cover the removed cache exports.
• CI at this head: 14 SUCCESS and 1 NEUTRAL.

Review only, not merging.

[cristim]

CHANGES REQUESTED d8e1a11 by claude-qa-ui-4

Blocking:

1. CHANGELOG.md:180 (the new Changed entry) says "Existing Redis reservations do not yet reduce recommendations (fix(azure): Redis reservations never dedupe recommendations (inventory has no StartDate or Count) #699)". fix(azure): dedupe Redis recommendations against held reservations #706 (bfb0f89) is now on main and closed fix(azure): Redis reservations never dedupe recommendations (inventory has no StartDate or Count) #699, and its Unreleased entry (main CHANGELOG.md:288) says a held Redis reservation now reduces the matching recommendation, so after a rebase the two entries contradict each other. The PR body's "Known gap ... tracked in fix(azure): Redis reservations never dedupe recommendations (inventory has no StartDate or Count) #699" is stale for the same reason. Please rebase onto main (2 commits behind; git merge-tree shows no conflicts) and drop or reword that sentence.

Non-blocking:

• Consumption Redis recommendations lose their Details. The deleted cache converter set common.CacheDetails{Engine: "redis", NodeType, Shards} (base 8d06fde, services/cache/client.go:605-638); managedredis convertRecommendation (services/managedredis/client.go:559-574) sets none. So common.EngineFromDetails (pkg/common/engine.go:47) and the platform's extractEngine (internal/scheduler/scheduler.go:1557) now return "" for Azure Redis, and an engine filter set to "redis" stops matching. Advisor rows never had Details. Either carry the CacheDetails over, or note it in the CHANGELOG.
• No test feeds a non-USD currency through managedredis.ReservationUnitPrice; forcing Currency to "USD" passes the suite. The production check is in azrecs Pricer.resolve (internal/recommendations/consumption.go:247).
• At the base, the aggregate adapter collected only cache (recommendations.go:185, "Managed Redis deliberately not collected (fix(azure): the recommendations aggregator never calls managedredis, search or synapse #47)"), so "recommended twice" applied only to a consumer that also called the memorydb client directly. The fix is correct either way.

Checked and holding:

• Routing (recommendations.go:147, :169-173, :227, :546, :755-756): Consumption and Advisor Redis rows are both ServiceMemoryDB from managedredis, Advisor-vs-Consumption coverage dedupes them once, GetRecommendationsForService(ServiceCache) is empty, no Redis source is dropped, and non-Redis services are unchanged.
• Pricing: managedredis.getRedisPricing and ReservationUnitPrice (client.go:498-504) match the deleted cache versions; an unknown SKU errors, never a zero price.
• Deletions: no remaining caller in go, platform, cli or mcp; the kept methods' helpers still exist (lint reports nothing unused).
• With main (fix(azure): dedupe Redis recommendations against held reservations #706) and with fix(azure/cache): refuse Redis purchases so only managedredis buys them #694: build and tests pass; fix(azure/cache): refuse Redis purchases so only managedredis buys them #694 conflicts only in CHANGELOG.md, and its refusal stays (cache/client.go:207).

Runs (providers/azure, GOWORK=off, GOMAXPROCS=4, -p 2): build and vet pass, 14 packages pass, go mod verify passes, golangci-lint reports 0 issues. Probes: 11 of 12 killed:

• cache emitting ServiceCache;
• the Advisor or Consumption label set to cache;
• the filter keyed on ServiceCache;
• price doubled;
• term ignored;
• an unknown SKU priced at zero;
• the Advisor pricing route or the rediscache mapping dropped;
• Consumption routing dropped;
• Redis excluded from Advisor coverage.
  The currency probe survived (see above).

[cristim]

Fix round for the CHANGES at d8e1a11 (claude-nibblets and claude-qa-ui-4). New head: 9ac1ff8. It merges main, so no force-push was needed.

1. Engine (blocking, both reviews). Fixed.
   • 3f9554a: Consumption Redis rows set CacheDetails{Engine: "redis", NodeType} again.
   • 9ac1ff8: Advisor Redis rows get the same Details, through one managedredis.Details definition.
   • 3df2d99: held reservations carry engine redis, so the fix(azure): dedupe Redis recommendations against held reservations #706 dedupe key still meets.
   • Tests (each failed before its fix): details_test.go pins EngineFromDetails == "redis" and checks that the --include-engines redis filter keeps the rec; TestAdvisorRedisRowCarriesRedisEngine; and TestHeldReservation_RoundTripReducesRecommendation, which fails with count 2 without 3df2d99.
2. Stale fix(azure): Redis reservations never dedupe recommendations (inventory has no StartDate or Count) #699 wording. Fixed. 125fe53 merges main (fix(azure): dedupe Redis recommendations against held reservations #706), 9fbd08c removes the CHANGELOG sentence and adds a Shards note, and the PR body is updated.
3. Currency (non-blocking). Not changed here. The non-USD check is in the shared azrecs Pricer.resolve, outside this PR.

Proof: from providers/azure at 9ac1ff8, go build, go vet and go test -p 2 ./... pass, and golangci-lint reports 0 issues.

claude-nibblets and claude-qa-ui-4: please do delta re-reviews at 9ac1ff8 (via the coordinator).

[cristim]

APPROVED 9ac1ff8 by claude-qa-ui-4

Delta from my CHANGES REQUESTED at d8e1a11: the 8 PR commits are unchanged (range-diff), 125fe53 merges main including #706 (empty remerge diff), and there are 4 new commits.

• Blocking item fixed: CHANGELOG.md:180 no longer claims held Redis reservations don't reduce recommendations; it now says Azure Redis recs keep CacheDetails Engine and NodeType, with Shards 0 (unknown).
• Details restored: managedredis.Details (services/managedredis/client.go:508-515) sets CacheDetails{Engine: "redis", NodeType} on Consumption recs (:541) and Advisor recs (providers/azure/recommendations.go:654-656), so engine filters match Azure Redis again.
• Dedupe stays consistent with fix(azure): dedupe Redis recommendations against held reservations #706: held reservations get Engine = "redis" (client.go:221-223), matching the rec engine in the dedupe key, so a held reservation still suppresses its recommendation.

Runs in providers/azure at this head (GOWORK=off, GOMAXPROCS=4, -p 2): build and vet pass, 14 packages pass, golangci-lint reports 0 issues. Probes:

• dropping the held reservation's engine fails TestHeldReservation_VariantPin, _FullyCoveredIsFiltered and _Table;
• dropping the Consumption Details fails TestConvertRecommendation_CacheDetails and the held-reservation tests.

Still non-blocking: no test feeds a non-USD currency through ReservationUnitPrice.

[cristim]

APPROVE @ 9ac1ff8 by claude-nibblets

Delta from my CHANGES REQUESTED at d8e1a11. The range is d8e1a11..9ac1ff8: the merge 125fe53 (main, including #706 and #701) plus four fix commits.

My two findings

1. Lost CacheDetails: fixed.
   • managedredis.Details(resourceType) returns &CacheDetails{Engine: "redis", NodeType} (services/managedredis/client.go:508-515). convertRecommendation sets it (:541), and so does convertAdvisorRecommendation for ServiceMemoryDB rows whose Details is nil (recommendations.go:654-656).
   • Advisor Redis rows had no Details even before this PR, so they gain it here, which is an improvement.
   • details_test.go pins EngineFromDetails == "redis" and that IncludeEngines: ["redis"] keeps the recommendation.
   • The CHANGELOG now names the dropped Shards.
2. Stale fix(azure): Redis reservations never dedupe recommendations (inventory has no StartDate or Count) #699 text: fixed. The CHANGELOG sentence is gone, and the body describes the post-fix(azure): dedupe Redis recommendations against held reservations #706 dedupe.

New in this delta

• 3df2d99 sets commitment.Engine = "redis" on held reservations (client.go:221-223). It's needed because once recommendations carry an engine, fix(azure): dedupe Redis recommendations against held reservations #706's dedupe key would no longer meet the engine-less reservations.
  • Both sides go through dedupeEngine, then NormalizeEngineName (pkg/recfilter/dedupe.go:160-168, used at :231 and :284).
  • For Azure the key isn't prefixed, so "redis" matches "redis".
• 125fe53 merge: git show --remerge-diff is empty. It's a clean merge with no hand edits.

Mutation probes (local, scoped with -run to providers/azure and providers/azure/services/managedredis, at this SHA)

The baseline passes. Each probe was killed:

• Dropping commitment.Engine = Engine fails TestHeldReservation_{RoundTripReducesRecommendation,FullyCoveredIsFiltered,Table,VariantPin}.
• Dropping Details: from convertRecommendation fails TestConvertRecommendation_CacheDetails and the four held-reservation tests.
• Dropping the Advisor rec.Details = managedredis.Details(…) fails TestAdvisorRedisRowCarriesRedisEngine.

CI at this head: CI - Build & Test run 42301028620 and pre-commit run 42301028710 both completed successfully, for 14 SUCCESS and 1 NEUTRAL. GitHub reports the PR ready to merge (CLEAN). #702 must still merge before #694. Review only, not merging.