Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions internal/config/recommendation_identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,19 @@ func RecIdentityKey(rec *RecommendationRecord) (string, error) {
}

// SuppressionVariant is the rec's availability zone, platform and tenancy as
// the purchase suppression key carries them (issue #816): the same AZ field and
// the same Details read (ComputeIdentity) as RecIdentityKey, so the write side
// (BuildSuppressions) and the read side (the scheduler's subtraction pass)
// cannot drift from the identity key or from each other.
// the purchase suppression key carries them (issue #816): the platform and
// tenancy come from the same Details read (ComputeIdentity) as RecIdentityKey,
// so the write side (BuildSuppressions) and the read side (the scheduler's
// subtraction pass) cannot drift from it or from each other. For AWS EC2 only,
// legacy tenancy "shared" is canonicalised to "default" and an AZ stored only
// in Details is used when the AZ field is empty (issue #1232).
func (r *RecommendationRecord) SuppressionVariant() (SuppressionVariant, error) {
ci, err := r.ComputeIdentity()
if err != nil {
return SuppressionVariant{}, err
}
v := SuppressionVariant{AvailabilityZone: r.AvailabilityZone, Platform: ci.Platform, Tenancy: ci.Tenancy}
if r.Provider == "aws" && CanonicalSuppressionService(r.Provider, r.Service) == "compute" {
if isAWSEC2(r.Provider, r.Service) {
v.Tenancy = canonicalSuppressionTenancy(v.Tenancy)
if v.AvailabilityZone == "" {
// Rows saved 2026-10-09..12 hold the zone only in Details (#1232).
Expand All @@ -86,23 +88,32 @@ func (r *RecommendationRecord) SuppressionVariant() (SuppressionVariant, error)
return v, nil
}

// isAWSEC2 is the scope of the legacy-shape canonicalisation: only AWS EC2
// ever stored tenancy "shared" or the zone only in Details.
func isAWSEC2(provider, service string) bool {
return provider == "aws" && CanonicalSuppressionService(provider, service) == "compute"
}

// canonicalSuppressionTenancy maps the pre-2026-05-20 EC2 tenancy spelling
// "shared" to today's "default" so old and new suppressions and recs match.
// common.NormalizeTenancyName is not used: it also case-folds and maps every
// unknown value to "", which would make distinct unknown tenancies match.
func canonicalSuppressionTenancy(t string) string {
if t == "shared" {
if strings.EqualFold(strings.TrimSpace(t), "shared") {
return "default"
}
return t
}

// detailsAvailabilityZone reads the zonal AZ from Details; "" when absent or
// unreadable (ComputeIdentity has already reported a decode error).
// unreadable, trimmed like the other Details-AZ readers. A non-string zone is
// treated as absent (regional), which can under-suppress; no producer writes one.
func (r *RecommendationRecord) detailsAvailabilityZone() string {
var d struct {
AvailabilityZone string `json:"availability_zone"`
}
if json.Unmarshal(r.Details, &d) != nil {
return ""
}
return d.AvailabilityZone
return strings.TrimSpace(d.AvailabilityZone)
}
4 changes: 1 addition & 3 deletions internal/config/suppressions_build.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package config
import (
"time"

"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
"github.com/LeanerCloud/cloud-commitments-go/pkg/logging"
)

Expand Down Expand Up @@ -43,8 +42,7 @@ func suppressionBucketOf(rec *RecommendationRecord) suppressionBucket {
b.unknownVariant = true
return b
}
isAWSEC2 := b.provider == string(common.ProviderAWS) && b.service == string(common.ServiceCompute)
if isAWSEC2 && v.Platform == "" && v.Tenancy == "" {
if isAWSEC2(b.provider, b.service) && v.Platform == "" && v.Tenancy == "" {
// Saved before Details existed (#453) and retried without re-matching:
// the bought platform/tenancy is unknown, so a known {az,"",""} would
// leave the real variant unsuppressed (double-buy). Wildcard instead.
Expand Down
42 changes: 39 additions & 3 deletions internal/config/suppressions_build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,43 @@ func TestBuildSuppressions_LegacyShapesCanonicalised(t *testing.T) {
}

func TestPurchaseSuppression_AppliesTo_LegacySharedTenancy(t *testing.T) {
stored := PurchaseSuppression{Variant: &SuppressionVariant{"us-east-1a", "Linux/UNIX", "shared"}}
assert.True(t, stored.AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}), "old stored shared row must match a default rec")
assert.False(t, stored.AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "dedicated"}))
def := &SuppressionVariant{"us-east-1a", "Linux/UNIX", "default"}
variant := &SuppressionVariant{"us-east-1a", "Linux/UNIX", "shared"}
for name, tc := range map[string]struct {
provider, service string
want bool
}{
"aws ec2 stored-side shared": {"aws", "ec2", true},
"aws compute slug": {"aws", "compute", true},
"aws rds untouched": {"aws", "rds", false},
"azure compute untouched": {"azure", "compute", false},
} {
t.Run(name, func(t *testing.T) {
stored := PurchaseSuppression{Provider: tc.provider, Service: tc.service, Variant: variant}
assert.Equal(t, tc.want, stored.AppliesTo(def))
})
}
ec2 := PurchaseSuppression{Provider: "aws", Service: "ec2", Variant: variant}
assert.False(t, ec2.AppliesTo(&SuppressionVariant{"us-east-1a", "Linux/UNIX", "dedicated"}))
}

func TestRecommendationRecord_SuppressionVariant_DetailsAZ(t *testing.T) {
for name, tc := range map[string]struct{ details, want string }{
"padded AZ is trimmed": {`{"availability_zone":" us-east-1a "}`, "us-east-1a"},
"non-string AZ is empty": {`{"availability_zone":5}`, ""},
} {
t.Run(name, func(t *testing.T) {
rec := RecommendationRecord{Provider: "aws", Service: "ec2", Details: []byte(tc.details)}
v, err := rec.SuppressionVariant()
require.NoError(t, err)
assert.Equal(t, tc.want, v.AvailabilityZone)
})
}
}

func TestCanonicalSuppressionTenancy(t *testing.T) {
assert.Equal(t, "default", canonicalSuppressionTenancy(" Shared "))
assert.Equal(t, "default", canonicalSuppressionTenancy("shared"))
assert.Equal(t, "Dedicated", canonicalSuppressionTenancy("Dedicated"))
assert.Equal(t, "default", canonicalSuppressionTenancy("default"))
}
6 changes: 5 additions & 1 deletion internal/config/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,11 @@ func (s *PurchaseSuppression) AppliesTo(v *SuppressionVariant) bool {
return true
}
a, b := *s.Variant, *v
a.Tenancy, b.Tenancy = canonicalSuppressionTenancy(a.Tenancy), canonicalSuppressionTenancy(b.Tenancy)
// Only the stored side needs canonicalising: SuppressionVariant already
// canonicalises every rec variant, so a raw "shared" never reaches v.
if isAWSEC2(s.Provider, s.Service) {
a.Tenancy = canonicalSuppressionTenancy(a.Tenancy)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return a == b
}

Expand Down
Loading