From 688db432dc351955cd0d91a3fc00d5c0807ba1ad Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 25 Apr 2026 14:20:04 +0200 Subject: [PATCH] chore(commitmentopts): MemoryDB probe target verification + page-cap test coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Combined follow-up to PR #54: addresses two issues surfaced during review that both touch internal/commitmentopts/. #62 — extract pagination loop into a shared walkPaginated helper The 6 per-service probers (RDS, ElastiCache, OpenSearch, Redshift, MemoryDB, EC2) all ran the same paginated-Describe loop body: page 0..N, fetch, accumulate, check next-page token, break/loop, with a hard maxPages=5 cap. The cap was previously exercised by a single TestRDSProber_PageCap test, leaving the other five probers' cap behaviour silently unverified — a refactor that broke one prober's loop in isolation could regress the cap without any test failing. Extracted the shared loop into walkPaginated(ctx, service, fetchPage) where fetchPage is a per-prober closure that handles each AWS API's specific quirks (Marker vs NextToken field name, per-item shape conversion, optional client-side instance-type filter for OpenSearch and Redshift). Error wrapping (": ") moves to the helper too. Chose extraction over mirroring TestRDSProber_PageCap five times because the loops were genuinely identical — only the per-item conversion differed, which is exactly what the closure now isolates. The new TestWalkPaginated_* tests cover page-cap, nil-token termination, empty-string-token termination, token threading across pages, error wrapping, and item accumulation in a single place. Per-prober Probe tests still exercise end-to-end wiring (which token field, per-item conversion, filters) so a wiring break in one prober is still caught. TestRDSProber_PageCap is retained as an integration smoke test. Closes #62. #61 — MemoryDB probe target db.t4g.small empirical risk Issue flagged that MemoryDB reserved-node coverage has historically skewed to db.r6g.* tiers, so db.t4g.small may return an empty offerings list in some regions, causing the cache to persist a probe run with zero MemoryDB combos. Resolution path was either: verify with AWS creds and switch to db.r6g.large if sparse, OR document the risk if no creds are available. This automated session had no AWS credentials (sts get-caller-identity NoCredentials), so the probe target stays db.t4g.small. Added a code comment in probe.go documenting the empirical risk, the verification command, and the safe fallback (db.r6g.large) so a future engineer with creds can finish the verification without re-discovering the context. A follow-up comment will be posted on issue #61 noting that AWS verification is still needed. Closes #61. Local verification: go build ./..., go vet ./..., go test ./... (all pass; commitmentopts package: ok 3.666s). --- internal/commitmentopts/probe.go | 169 +++++++++++++++++--------- internal/commitmentopts/probe_test.go | 105 +++++++++++++++- 2 files changed, 212 insertions(+), 62 deletions(-) diff --git a/internal/commitmentopts/probe.go b/internal/commitmentopts/probe.go index b293a65f..fd6e2f03 100644 --- a/internal/commitmentopts/probe.go +++ b/internal/commitmentopts/probe.go @@ -29,6 +29,19 @@ const pageSize int32 = 100 // long-standing public availability. The targets never round-trip through // a purchase so their cost is irrelevant; what matters is that AWS // actually has offerings to return for them. +// +// probeTargetMemoryDB carries an empirical risk: MemoryDB reserved-node +// coverage has historically skewed to db.r6g.* tiers, and db.t4g.small may +// return an empty offerings list in some regions. If that happens the +// orchestrating Service still persists the run (so we don't re-probe in a +// hot loop) and the frontend silently falls back to hardcoded MemoryDB +// rules. Switching to db.r6g.large would be the safe alternative once a +// human with AWS creds confirms via: +// +// aws memorydb describe-reserved-nodes-offerings \ +// --region us-east-1 --node-type db.t4g.small +// +// Tracked in github.com/LeanerCloud/CUDly#61. const ( probeTargetRDS = "db.t3.micro" probeTargetElastiCache = "cache.t3.micro" @@ -38,6 +51,48 @@ const ( probeTargetEC2 = "t3.micro" ) +// walkPaginated runs the standard paginated-Describe loop shared by every +// per-service prober: call fetchPage, accumulate the rawOffers it yields, +// thread the next-page token, and stop after maxPages iterations even if +// AWS keeps returning a non-empty token. Each prober supplies its own +// fetchPage closure that handles per-API quirks (Marker vs NextToken, +// per-item shape conversion, optional client-side instance-type filter). +// +// The loop is broken out into a single helper so the page cap is exercised +// by one unit test (TestWalkPaginated_StopsAtPageCap) rather than six +// near-duplicate tests, and so a refactor can't silently lose the cap on +// one prober while leaving the others intact. +// +// fetchPage receives the token from the previous page (nil on the first +// call) and returns: +// - the rawOffers extracted from this page +// - the next-page token (nil or "" means "no more pages") +// - any API error +// +// service is the canonical service name; it's used to wrap fetchPage errors +// as ": " so callers see which prober blew up without each +// one having to repeat the wrap. +func walkPaginated( + ctx context.Context, + service string, + fetchPage func(ctx context.Context, token *string) ([]rawOffer, *string, error), +) ([]rawOffer, error) { + var raw []rawOffer + var token *string + for page := 0; page < maxPages; page++ { + offers, next, err := fetchPage(ctx, token) + if err != nil { + return nil, fmt.Errorf("%s: %w", service, err) + } + raw = append(raw, offers...) + if next == nil || aws.ToString(next) == "" { + break + } + token = next + } + return raw, nil +} + // collect dedupes a probe's raw (durationSeconds, rawPayment) pairs, runs // both normalizers, and builds the Combo slice. Duplicates — a single // (term, payment) tuple appears once per instance size × AZ × engine @@ -107,27 +162,26 @@ func (p *RDSProber) Service() string { return "rds" } // against db.t3.micro. func (p *RDSProber) Probe(ctx context.Context, cfg aws.Config) ([]Combo, error) { client := p.client(cfg) - var raw []rawOffer - var marker *string - for page := 0; page < maxPages; page++ { + raw, err := walkPaginated(ctx, p.Service(), func(ctx context.Context, token *string) ([]rawOffer, *string, error) { out, err := client.DescribeReservedDBInstancesOfferings(ctx, &rds.DescribeReservedDBInstancesOfferingsInput{ DBInstanceClass: aws.String(probeTargetRDS), MaxRecords: aws.Int32(pageSize), - Marker: marker, + Marker: token, }) if err != nil { - return nil, fmt.Errorf("rds: %w", err) + return nil, nil, err } + offers := make([]rawOffer, 0, len(out.ReservedDBInstancesOfferings)) for _, o := range out.ReservedDBInstancesOfferings { - raw = append(raw, rawOffer{ + offers = append(offers, rawOffer{ durationSeconds: int64(aws.ToInt32(o.Duration)), payment: aws.ToString(o.OfferingType), }) } - if out.Marker == nil || aws.ToString(out.Marker) == "" { - break - } - marker = out.Marker + return offers, out.Marker, nil + }) + if err != nil { + return nil, err } return collect(p.Service(), raw), nil } @@ -159,27 +213,26 @@ func (p *ElastiCacheProber) Service() string { return "elasticache" } // Probe returns the combos for cache.t3.micro. func (p *ElastiCacheProber) Probe(ctx context.Context, cfg aws.Config) ([]Combo, error) { client := p.client(cfg) - var raw []rawOffer - var marker *string - for page := 0; page < maxPages; page++ { + raw, err := walkPaginated(ctx, p.Service(), func(ctx context.Context, token *string) ([]rawOffer, *string, error) { out, err := client.DescribeReservedCacheNodesOfferings(ctx, &elasticache.DescribeReservedCacheNodesOfferingsInput{ CacheNodeType: aws.String(probeTargetElastiCache), MaxRecords: aws.Int32(pageSize), - Marker: marker, + Marker: token, }) if err != nil { - return nil, fmt.Errorf("elasticache: %w", err) + return nil, nil, err } + offers := make([]rawOffer, 0, len(out.ReservedCacheNodesOfferings)) for _, o := range out.ReservedCacheNodesOfferings { - raw = append(raw, rawOffer{ + offers = append(offers, rawOffer{ durationSeconds: int64(aws.ToInt32(o.Duration)), payment: aws.ToString(o.OfferingType), }) } - if out.Marker == nil || aws.ToString(out.Marker) == "" { - break - } - marker = out.Marker + return offers, out.Marker, nil + }) + if err != nil { + return nil, err } return collect(p.Service(), raw), nil } @@ -213,29 +266,28 @@ func (p *OpenSearchProber) Service() string { return "opensearch" } // Probe returns the combos for t3.small.search. func (p *OpenSearchProber) Probe(ctx context.Context, cfg aws.Config) ([]Combo, error) { client := p.client(cfg) - var raw []rawOffer - var nextToken *string - for page := 0; page < maxPages; page++ { + raw, err := walkPaginated(ctx, p.Service(), func(ctx context.Context, token *string) ([]rawOffer, *string, error) { out, err := client.DescribeReservedInstanceOfferings(ctx, &opensearch.DescribeReservedInstanceOfferingsInput{ MaxResults: pageSize, - NextToken: nextToken, + NextToken: token, }) if err != nil { - return nil, fmt.Errorf("opensearch: %w", err) + return nil, nil, err } + offers := make([]rawOffer, 0, len(out.ReservedInstanceOfferings)) for _, o := range out.ReservedInstanceOfferings { if string(o.InstanceType) != probeTargetOpenSearch { continue } - raw = append(raw, rawOffer{ + offers = append(offers, rawOffer{ durationSeconds: int64(o.Duration), payment: string(o.PaymentOption), }) } - if out.NextToken == nil || aws.ToString(out.NextToken) == "" { - break - } - nextToken = out.NextToken + return offers, out.NextToken, nil + }) + if err != nil { + return nil, err } return collect(p.Service(), raw), nil } @@ -269,29 +321,28 @@ func (p *RedshiftProber) Service() string { return "redshift" } // Probe returns the combos for dc2.large. func (p *RedshiftProber) Probe(ctx context.Context, cfg aws.Config) ([]Combo, error) { client := p.client(cfg) - var raw []rawOffer - var marker *string - for page := 0; page < maxPages; page++ { + raw, err := walkPaginated(ctx, p.Service(), func(ctx context.Context, token *string) ([]rawOffer, *string, error) { out, err := client.DescribeReservedNodeOfferings(ctx, &redshift.DescribeReservedNodeOfferingsInput{ MaxRecords: aws.Int32(pageSize), - Marker: marker, + Marker: token, }) if err != nil { - return nil, fmt.Errorf("redshift: %w", err) + return nil, nil, err } + offers := make([]rawOffer, 0, len(out.ReservedNodeOfferings)) for _, o := range out.ReservedNodeOfferings { if aws.ToString(o.NodeType) != probeTargetRedshift { continue } - raw = append(raw, rawOffer{ + offers = append(offers, rawOffer{ durationSeconds: int64(aws.ToInt32(o.Duration)), payment: aws.ToString(o.OfferingType), }) } - if out.Marker == nil || aws.ToString(out.Marker) == "" { - break - } - marker = out.Marker + return offers, out.Marker, nil + }) + if err != nil { + return nil, err } return collect(p.Service(), raw), nil } @@ -323,27 +374,26 @@ func (p *MemoryDBProber) Service() string { return "memorydb" } // Probe returns the combos for db.t4g.small. func (p *MemoryDBProber) Probe(ctx context.Context, cfg aws.Config) ([]Combo, error) { client := p.client(cfg) - var raw []rawOffer - var nextToken *string - for page := 0; page < maxPages; page++ { + raw, err := walkPaginated(ctx, p.Service(), func(ctx context.Context, token *string) ([]rawOffer, *string, error) { out, err := client.DescribeReservedNodesOfferings(ctx, &memorydb.DescribeReservedNodesOfferingsInput{ NodeType: aws.String(probeTargetMemoryDB), MaxResults: aws.Int32(pageSize), - NextToken: nextToken, + NextToken: token, }) if err != nil { - return nil, fmt.Errorf("memorydb: %w", err) + return nil, nil, err } + offers := make([]rawOffer, 0, len(out.ReservedNodesOfferings)) for _, o := range out.ReservedNodesOfferings { - raw = append(raw, rawOffer{ + offers = append(offers, rawOffer{ durationSeconds: int64(o.Duration), payment: aws.ToString(o.OfferingType), }) } - if out.NextToken == nil || aws.ToString(out.NextToken) == "" { - break - } - nextToken = out.NextToken + return offers, out.NextToken, nil + }) + if err != nil { + return nil, err } return collect(p.Service(), raw), nil } @@ -378,28 +428,27 @@ func (p *EC2Prober) Service() string { return "ec2" } // normalization. func (p *EC2Prober) Probe(ctx context.Context, cfg aws.Config) ([]Combo, error) { client := p.client(cfg) - var raw []rawOffer - var nextToken *string - for page := 0; page < maxPages; page++ { + raw, err := walkPaginated(ctx, p.Service(), func(ctx context.Context, token *string) ([]rawOffer, *string, error) { out, err := client.DescribeReservedInstancesOfferings(ctx, &ec2.DescribeReservedInstancesOfferingsInput{ InstanceType: ec2types.InstanceType(probeTargetEC2), IncludeMarketplace: aws.Bool(false), MaxResults: aws.Int32(pageSize), - NextToken: nextToken, + NextToken: token, }) if err != nil { - return nil, fmt.Errorf("ec2: %w", err) + return nil, nil, err } + offers := make([]rawOffer, 0, len(out.ReservedInstancesOfferings)) for _, o := range out.ReservedInstancesOfferings { - raw = append(raw, rawOffer{ + offers = append(offers, rawOffer{ durationSeconds: aws.ToInt64(o.Duration), payment: string(o.OfferingType), }) } - if out.NextToken == nil || aws.ToString(out.NextToken) == "" { - break - } - nextToken = out.NextToken + return offers, out.NextToken, nil + }) + if err != nil { + return nil, err } return collect(p.Service(), raw), nil } diff --git a/internal/commitmentopts/probe_test.go b/internal/commitmentopts/probe_test.go index fd66dba7..88798b92 100644 --- a/internal/commitmentopts/probe_test.go +++ b/internal/commitmentopts/probe_test.go @@ -95,8 +95,11 @@ func TestRDSProber_ErrorPropagates(t *testing.T) { } func TestRDSProber_PageCap(t *testing.T) { - // Every page returns a non-empty marker; the probe must stop after - // maxPages to bound API spend. + // Integration-level check that the RDS prober honours the page cap + // when wired through walkPaginated. The cap itself is exercised in + // detail by TestWalkPaginated_StopsAtPageCap; this test guards the + // wiring (RDS uses Marker rather than NextToken) so a refactor that + // stops threading the marker can't silently break only RDS. calls := 0 fake := &fakeRDS{fn: func(*rds.DescribeReservedDBInstancesOfferingsInput) (*rds.DescribeReservedDBInstancesOfferingsOutput, error) { calls++ @@ -344,3 +347,101 @@ func TestDefaultProbers(t *testing.T) { sort.Strings(services) assert.Equal(t, []string{"ec2", "elasticache", "memorydb", "opensearch", "rds", "redshift"}, services) } + +// --------------------------------------------------------------------------- +// walkPaginated — the shared pagination helper every prober runs through. +// Testing the helper once covers the page-cap behaviour for all six +// services in lieu of six near-identical Test{Service}Prober_PageCap tests. +// The per-prober Probe tests above still exercise the wiring (which token +// field each AWS API uses, per-item conversion, optional client-side +// filtering) end-to-end. +// --------------------------------------------------------------------------- + +func TestWalkPaginated_StopsAtPageCap(t *testing.T) { + // Every page returns a non-empty token; walkPaginated must stop after + // maxPages calls to bound worst-case API spend if pagination + // detection is ever broken (SDK regression, malformed AWS response). + calls := 0 + _, err := walkPaginated(context.Background(), "test", func(ctx context.Context, token *string) ([]rawOffer, *string, error) { + calls++ + return nil, aws.String("more"), nil + }) + require.NoError(t, err) + assert.Equal(t, maxPages, calls) +} + +func TestWalkPaginated_StopsOnNilToken(t *testing.T) { + // AWS signals "no more pages" with a nil token — the helper must + // stop on the first page that returns one rather than keep looping. + calls := 0 + got, err := walkPaginated(context.Background(), "test", func(ctx context.Context, token *string) ([]rawOffer, *string, error) { + calls++ + return []rawOffer{{durationSeconds: 31536000, payment: "All Upfront"}}, nil, nil + }) + require.NoError(t, err) + assert.Equal(t, 1, calls) + assert.Len(t, got, 1) +} + +func TestWalkPaginated_StopsOnEmptyToken(t *testing.T) { + // Some AWS APIs return an empty (non-nil) string instead of nil to + // signal "done" — the helper must treat both as equivalent. + calls := 0 + _, err := walkPaginated(context.Background(), "test", func(ctx context.Context, token *string) ([]rawOffer, *string, error) { + calls++ + return nil, aws.String(""), nil + }) + require.NoError(t, err) + assert.Equal(t, 1, calls) +} + +func TestWalkPaginated_ThreadsTokenAcrossPages(t *testing.T) { + // The helper must pass each page's returned token back as the next + // page's input token so the AWS SDK can resume from the right cursor. + tokens := []string{"", "page1", "page2"} + got := []string{} + calls := 0 + _, err := walkPaginated(context.Background(), "test", func(ctx context.Context, token *string) ([]rawOffer, *string, error) { + got = append(got, aws.ToString(token)) + calls++ + if calls >= len(tokens) { + return nil, nil, nil + } + return nil, aws.String(tokens[calls]), nil + }) + require.NoError(t, err) + assert.Equal(t, []string{"", "page1", "page2"}, got) +} + +func TestWalkPaginated_WrapsErrorWithService(t *testing.T) { + // Errors from fetchPage must be wrapped as ": " so + // callers see which prober failed without each one repeating the + // wrap. errors.Is must still find the underlying error. + boom := errors.New("boom") + _, err := walkPaginated(context.Background(), "memorydb", func(ctx context.Context, token *string) ([]rawOffer, *string, error) { + return nil, nil, boom + }) + require.Error(t, err) + assert.ErrorIs(t, err, boom) + assert.Contains(t, err.Error(), "memorydb:") +} + +func TestWalkPaginated_AccumulatesAcrossPages(t *testing.T) { + // Items from each page should accumulate into a single slice in + // page order — collect() does the dedupe, walkPaginated does not. + calls := 0 + got, err := walkPaginated(context.Background(), "test", func(ctx context.Context, token *string) ([]rawOffer, *string, error) { + calls++ + offers := []rawOffer{{durationSeconds: int64(calls), payment: "All Upfront"}} + if calls >= 3 { + return offers, nil, nil + } + return offers, aws.String("more"), nil + }) + require.NoError(t, err) + assert.Equal(t, 3, calls) + require.Len(t, got, 3) + assert.Equal(t, int64(1), got[0].durationSeconds) + assert.Equal(t, int64(2), got[1].durationSeconds) + assert.Equal(t, int64(3), got[2].durationSeconds) +}