fix(recfilter): match EC2 commitments on platform, tenancy and scope when deduplicating by cristim · Pull Request #460 · LeanerCloud/cloud-commitments-go

cristim · 2026-10-16T18:43:48Z

Summary

This is PR 1b of the first #323 slice. It is stacked on #458 (PR 1a), so merge #458 first, then retarget this PR to main. CI only runs on PRs to main.

The recfilter duplicate check keyed AWS EC2 coverage on instance type and region only. A recent Windows, dedicated or zonal reservation therefore counted as covering a Linux, default-tenancy or regional recommendation. This PR adds the dimensions that rule out coverage, whatever the RI sharing setting. The new code is in pkg/recfilter/dedupe_ec2.go; dedupe.go gets a small hook.

Gating. The new matching applies only when Provider is AWS and Service is EC2 or Compute, checked on both the commitment and the recommendation side. Every other service keeps today's key and behaviour.

Platform and tenancy. Values must be equal after NormalizePlatformName / NormalizeTenancyName, or else empty on either side. An empty value is a wildcard that still covers.

Scope. Each side gets one of three states: zonal(az), regional, or unknown. scopeCovers(entry, rec) implements this truth table, mirrored row by row in TestScopeCovers:

a regional RI covers regional, zonal(any) and unknown recs;
zonal(a) covers zonal(a) and unknown, but not zonal(b) or regional;
a zonal entry with an empty AZ covers zonal(any) and unknown, but not regional;
an unknown entry covers anything;
an unknown rec is covered by any entry.

Matching. Each base key holds a stable-ordered slice of entries, sorted by wildcard count and then input order. No map iteration. A rec consumes exact matches before wildcards. Partial consumption is summed, and common.ResizeCount is called once, so money scales with the count.

Unrecognized values.

Tenancy and scope values that don't normalize log one WARN and act as a wildcard.
Platform values outside the known EC2 platforms log one WARN and match only the same platform.

Account is deliberately not keyed. A commitment in another linked account still suppresses, and a test pins that. Under RI sharing, the Organizations default, keying on account would let a duplicate through. See the design note on #323.

PR 2 has the EC2 listing (GetExistingCommitments) fill these fields. Until then, real EC2 commitments carry empty dimensions and behave as wildcards, which is exactly today's behaviour. Size-flexibility is out of scope.

Refs #323

Verification (macOS, Go 1.26.9)

Build and lint

(cd pkg && GOWORK=off go test -race -short ./...) passes.
make build and make vet pass.
golangci-lint v2.10.1 (the CI pin) reports 0 issues.
GOWORK=off go build ./... passes in all five modules.
gocyclo is clean.

Tests

TestScopeCovers.
TestEC2DedupeMatching:
- Windows does not cover Linux, dedicated does not cover default, zonal does not cover regional or another zone, and regional covers zonal.
- Positive controls: "Linux/UNIX (Amazon VPC)" matches "Linux/UNIX", "shared" matches "default", and "Region" matches "regional".
- Wildcards on each side, and a commitment in a different account still suppresses.
TestEC2DedupeValueDetails (value and typed-nil details).
TestEC2DedupeAzureNotKeyed.
TestEC2DedupeWildcardPreference: determinism, and a count-5 rec against 2 exact + 2 wildcard entries with money scaled for 1.
TestEC2DedupeEntryOrder: exact matches are consumed before wildcards, and input order breaks ties.
WARN tests, including no warning for "Linux with SQL Server".

Fail-before. On 1a's code these fail: Windows vs Linux, dedicated vs default, zonal vs regional, the other-zone case, the wildcard preference test and the WARN test.

Mutation probes. The independent reviewer ran these in scratch copies, and each one fails a test:

flip scopeCovers, or let a zonal-empty-AZ entry cover regional;
drop the platform, tenancy or AZ check;
call ResizeCount per entry;
reverse the sort, or reverse equal-wildcard input order.

Review. The plan went through two passes plus truth-table iterations with a non-author reviewer. The same reviewer did three diff passes. Pass 1 found the vacuous ordering test, a missing platform WARN and an unneeded skip. Pass 2 found the SQL Server prefix and the WARN wording. No actionable findings remained.

Unit tests with constructed commitments and recommendations only. No cloud call.

🤖 Generated with Claude Code

coderabbitai · 2026-10-16T18:43:58Z

Warning

Review limit reached

Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 11 seconds for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 66 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
Review profile: CHILL
Plan: Essentials
Run ID: 589c7f37-26e2-4e15-ae57-6983b1c06ec2

📥 Commits

Reviewing files that changed from the base of the PR and between 9260573 and ae714be.

📒 Files selected for processing: 3

pkg/recfilter/dedupe.go
pkg/recfilter/dedupe_ec2.go
pkg/recfilter/dedupe_ec2_test.go

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

_{Comment @coderabbitai help to get the list of available commands.}

cristim · 2026-10-16T18:47:28Z

Independent review verdict: CLEAN at `18d68a9`

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Scope: the one commit on top of #458 (64c9741).

Blocking findings: none. The duplicate-purchase direction was the focus: an owned RI that stops counting is re-bought.

Behaviour is unchanged today.

providers/aws/services/ec2/client.go:79-123 (GetExistingCommitments) does not populate Platform, Tenancy, Scope or AZ, and nothing on main does.
So every real EC2 entry is all "", which is a full wildcard. It covers the same type and region as before.
Count handling is unchanged.

When PR 2 populates the fields, the same real RI and recommendation normalize equal on every dimension:

Dimension	Existing RI (SDK)	Rec (CE `parser_services.go:159-172`)	Normalized equal
Platform	"Linux/UNIX (Amazon VPC)" / "Linux/UNIX"	"Linux/UNIX"	Yes: `linux/unix` (`engine.go:79-85`). Windows, RHEL, SUSE and SQL variants are equal too.
Tenancy	"default"	`resolveEC2Tenancy` "default" (nil maps to default); "shared" also maps to default	Yes. dedicated and host stay distinct.
Scope	"Region" / "Availability Zone"	`resolveEC2Scope` same, or regional/zonal	Yes: regional/zonal
AZ	lowercased and trimmed	lowercased and trimmed	Yes
Empty vs set	wildcard		Covers

Unrecognized tenancy or scope normalizes to "" (covers), so it fails toward no re-buy. An unrecognized platform requires the identical string, and both sides come from AWS.

Gating and scope coverage:

isAWSCompute (AWS plus EC2 or Compute) gates both the entry and the rec. Every other service and provider takes the unchanged path, and a test pins Azure Compute as excluded.
scopeCovers: a regional entry covers regional, zonal and unknown recs. A zonal entry does not cover a regional rec, which is the intended fix. Tests pin each case.

What was attacked and how (pkg, GOWORK=off, go1.26.9):

go vet ./... is clean. go test -race -count=1 ./... passes 1696 in 15 packages; recfilter passes 250.
Mutation probes, both killed:
- dropping the scope comparison fails TestEC2DedupeMatching and EntryOrder;
- making "" not a wildcard fails ValueDetails, EntryOrder, Matching and WildcardPreference.
Fail-before: reverting the two files fails to compile, because the tests use the new symbols. The old behaviour over-matches by design.

Gate:

Stacked on feat(common): add EC2 platform, tenancy and scope dimensions to Commitment #458. After feat(common): add EC2 platform, tenancy and scope dimensions to Commitment #458 merges, retarget to main and rebase. A range-diff must show the commit unchanged, then CI must be green.
For PR 2, which populates the fields: re-check the table against the real DescribeReservedInstances mapping.

…when deduplicating AWS EC2 and compute commitments were deduplicated by region and instance type alone, so a Windows or dedicated reservation suppressed a Linux or default-tenancy recommendation. They now match on normalized platform and tenancy (empty on either side is a wildcard) and on scope. Scope is asymmetric: a regional reservation covers regional and zonal demand, a zonal one covers only its own zone, and an unknown scope on either side is a wildcard. Entries are kept in a stable order, fewest wildcards consumed first, and a recommendation is resized once to what remains. Account stays out of the key, so a commitment in another account still suppresses. Every other service keeps its existing key. Refs #323 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

cristim · 2026-10-16T18:54:31Z

Retargeted to main after #458 squash-merged as 9260573, then rebased with git rebase --onto origin/main 64c97419. git range-diff of 18d68a9 against new head ae714be shows the commit as identical (=), so the CLEAN verdict carries to ae714be. (claude-cc-reviewer)

cristim · 2026-10-16T19:39:28Z

CI is green on head ae714be5 after the retarget to main: pre-commit 38950938061 and Build & Test 38950937466.

cristim · 2026-10-16T19:39:29Z

@coderabbitai review

coderabbitai · 2026-10-16T19:39:37Z

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim added priority/p2 Backlog-worthy severity/medium Moderate harm effort/l Weeks urgency/this-quarter Within the quarter impact/many Affects most users type/bug Defect triaged Item has been triaged labels Oct 16, 2026

cristim force-pushed the fix/323-dedupe-ec2-account-dims branch from 64c9741 to 30e042d Compare October 16, 2026 18:44

cristim changed the base branch from fix/323-dedupe-ec2-account-dims to main October 16, 2026 18:54

cristim force-pushed the fix/323-dedupe-ec2-matching branch from 18d68a9 to ae714be Compare October 16, 2026 18:54

cristim merged commit 89772cf into main Oct 16, 2026
15 checks passed

cristim deleted the fix/323-dedupe-ec2-matching branch October 16, 2026 19:40

Navigation Menu

Repository navigation

fix(recfilter): match EC2 commitments on platform, tenancy and scope when deduplicating - #460

fix(recfilter): match EC2 commitments on platform, tenancy and scope when deduplicating#460
cristim merged 1 commit into
mainfrom
fix/323-dedupe-ec2-matching

cristim commented Oct 16, 2026

Uh oh!

coderabbitai Bot commented Oct 16, 2026 •

edited

Loading

Review limit reached

Uh oh!

cristim commented Oct 16, 2026

Uh oh!

cristim commented Oct 16, 2026

Uh oh!

cristim commented Oct 16, 2026

Uh oh!

cristim commented Oct 16, 2026

Uh oh!

coderabbitai Bot commented Oct 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

cristim commented Oct 16, 2026

Summary

Verification (macOS, Go 1.26.9)

Uh oh!

coderabbitai Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

cristim commented Oct 16, 2026

Independent review verdict: CLEAN at 18d68a9

Uh oh!

cristim commented Oct 16, 2026

Uh oh!

cristim commented Oct 16, 2026

Uh oh!

cristim commented Oct 16, 2026

Uh oh!

coderabbitai Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Oct 16, 2026 •

edited

Loading

Independent review verdict: CLEAN at `18d68a9`

coderabbitai Bot commented Oct 16, 2026 •

edited

Loading