feat(azure compute): populate vCPU + MemoryGB via cached SKU catalogue (closes #148)#229
Conversation
closes #148) PR #97 added VCPU + MemoryGB fields to common.ComputeDetails (with omitempty JSON tags). PR #81 introduced the lazy SKU-catalogue pattern for cache/cosmosdb/database but explicitly scoped back compute because the destination fields didn't exist. Both prerequisites are now in place; this wires Azure compute to the same pattern. Implementation mirrors cache/database verbatim for consistency: - New unexported vmSKUEntry{vCPUs, memoryGB}. - New skuCacheOnce sync.Once + skuCacheMap field on ComputeClient. - cachedSKULookup(ctx, skuName) lazily triggers the catalogue fetch on first call; subsequent calls are O(1) map lookups. ok=false on miss or fetch error. - fetchSKUCatalogue reuses the existing createResourceSKUsPager and isAvailableInRegion helpers (already used by GetValidResourceTypes), walks every page, and reduces virtualMachines SKUs into the map. Returns nil on error so the sync.Once-gated cache stays nil and converters fall back to the empty-fields path with a one-time WARN. - extractVMSKUCapabilities pulls vCPUs (Atoi) and MemoryGB (ParseFloat) from the SKU's Capabilities name/value list. Unparseable or missing capabilities → 0, treated as "unknown". - populateVMSKUMapFromPage was extracted out of fetchSKUCatalogue to stay under the project's gocyclo=10 threshold (matches the cache/database extraction pattern from PR #81). - convertAzureVMRecommendation now takes ctx (was _) and populates Details.VCPU/MemoryGB from the cache when both >0. The single caller GetRecommendations already passes ctx; no public API change. Behaviour preserved on failure: a transient ResourceSKUsClient error no longer breaks the conversion — VCPU/MemoryGB stay at 0 (omitempty hides them from API payloads), the rest of Details is populated from the recommendation payload as before, and a WARN is logged once per client lifetime. UX improvement: common.ComputeDetails.GetDetailDescription appends " (<vcpu> vCPU / <memory> GB)" when both fields are >0, so Azure VM recommendation summaries now include the size string instead of the SKU name alone. Tests added in providers/azure/services/compute/client_test.go (file-scoped vmSKUCatalogueMockPager keeps the shared mocks.MockResourceSKUsPager surface untouched, mirroring the cosmosdb / cache test convention): - TestComputeClient_ConvertAzureVMRecommendation_PopulatesVCPUAndMemoryFromSKUCache: catalogue hit populates VCPU=2, MemoryGB=8 for Standard_D2s_v3. - TestComputeClient_ConvertAzureVMRecommendation_PagerErrorFallsBack: fetch error leaves both fields at 0; conversion succeeds. - TestComputeClient_ConvertAzureVMRecommendation_NoMatchLeavesFieldsZero: catalogue miss leaves both fields at 0; conversion succeeds. - TestComputeClient_CachedSKULookup_FetchedOnce: 10 lookups trigger exactly 1 NextPage call (pins the N+1 invariant per PR #81). Out of scope (separate follow-ups, per the issue body): - AWS / GCP compute converter wiring. - Platform / Tenancy / Scope enrichment (require non-ResourceSKUs Azure data sources). Verification: gofmt clean; go vet ./... clean; gocyclo -over 10 finds no offenders in the modified file; go test github.com/LeanerCloud/CUDly/providers/azure/services/compute → 42 passing (4 new + all existing). The pre-existing providers/azure/services/search build failure (missing go.sum entry) is unrelated to this change. Closes #148
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAzure Compute recommendations are enriched with vCPU and MemoryGB data via a lazily-cached SKU catalogue. The ComputeClient fetches ARM Resource SKUs once per lifetime, parses capabilities, and the recommendation converter populates ComputeDetails from the cache with graceful fallback to zero on failures. ChangesAzure SKU-Based Compute Enrichment
Sequence DiagramsequenceDiagram
participant Converter as convertAzureVMRecommendation
participant Client as ComputeClient
participant Cache as SKU Cache<br/>(sync.Once)
participant Pager as ResourceSKUsClient<br/>Pager
participant ARM as Azure SKUs<br/>API
Note over Converter,ARM: First recommendation conversion
Converter->>Client: cachedSKULookup(vmName)
Client->>Cache: Check cache (sync.Once gate)
alt Cache not yet initialized
Cache->>Pager: fetchSKUCatalogue()
Pager->>ARM: ListByLocation(region) paginated
ARM-->>Pager: SKU pages
Pager->>Cache: Build name→{vCPU, MemoryGB} map
Cache-->>Client: Return (entry, hit)
else Cache already initialized
Cache-->>Client: Return (entry, hit)
end
Client-->>Converter: {vCPUs, MemoryGB}
Converter->>Converter: Populate ComputeDetails
Note over Converter,ARM: Second recommendation (same client)
Converter->>Client: cachedSKULookup(otherVmName)
Client->>Cache: Check cache (sync.Once skipped)
Cache-->>Client: Return from map (no API call)
Client-->>Converter: {vCPUs, MemoryGB}
Converter->>Converter: Populate ComputeDetails
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 2/5 reviews remaining, refill in 29 minutes and 19 seconds. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
) A fork that pushes a PR, pings CodeRabbit, then exits leaves CR threads unresolved — exactly what happened to PRs #228, #229, #230, etc., where CR posted Actionable findings + Nitpicks that were never triaged. This adds a paragraph to the CR-loop section explicitly addressing the delegation case: subagent prompts MUST include the full loop with the exit criteria spelled out. Stops the failure mode where the rule is correctly stated for humans but doesn't get mirrored into fork prompts.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
Wires Azure compute to the lazy SKU-catalogue lookup pattern from PR #81, so
convertAzureVMRecommendationnow populatescommon.ComputeDetails.VCPUandcommon.ComputeDetails.MemoryGBfromarmcompute.ResourceSKUsClient.NewListPagercapabilities. Both prerequisites are now in place: PR #97 added the destination fields (withomitemptyJSON tags), and PR #81 landed the cache pattern for the other three Azure services.Closes #148.
Why
common.ComputeDetails.GetDetailDescription()already readsVCPUandMemoryGBand appends" (<vcpu> vCPU / <memory> GB)"when both are >0. Pre-PR, Azure VM recommendations always had both at 0, so the size string was suppressed and the UI rendered SKU names alone (e.g.Standard_D2s_v3). Post-PR they render asStandard_D2s_v3 (2 vCPU / 8 GB).What changed
providers/azure/services/compute/client.go:vmSKUEntry{vCPUs, memoryGB}.skuCacheOnce sync.Once+skuCacheMap map[string]vmSKUEntryonComputeClient.cachedSKULookup(ctx, skuName)— lazy single-fetch, O(1) lookup thereafter.fetchSKUCatalogue(ctx)— reuses the existingcreateResourceSKUsPagerandisAvailableInRegionhelpers (already used byGetValidResourceTypes); returns nil on error.populateVMSKUMapFromPageextracted out offetchSKUCatalogueto stay under the project'sgocyclo=10threshold (matches the cache/database extraction pattern from PR perf(azure): batched SKU catalogue lookup eliminates N+1 in recommendation converters #81).extractVMSKUCapabilitiesparsesvCPUs(Atoi) andMemoryGB(ParseFloat) from the SKU's name/value capability list.convertAzureVMRecommendationtakesctx(was_) and populatesDetails.VCPU/MemoryGBfrom the cache when both >0. Single callerGetRecommendationsalready passesctx.providers/azure/services/compute/client_test.go: 4 new tests_PopulatesVCPUAndMemoryFromSKUCache— catalogue hit → fields enriched._PagerErrorFallsBack— fetch error → fields stay 0; conversion succeeds._NoMatchLeavesFieldsZero— catalogue miss → fields stay 0._CachedSKULookup_FetchedOnce— 10 lookups → 1 NextPage call (pins the N+1 invariant).The shared
mocks.MockResourceSKUsPagerdoesn't expose its page counter and can't simulate aNextPageerror, so the tests use a small file-scopedvmSKUCatalogueMockPager(matches the cosmosdb/cache test convention where each service defines its own catalogue mock). No change to the shared mocks surface.Graceful-degradation contract
A transient
ResourceSKUsClientfailure no longer breaks Azure VM recommendation conversion:VCPU/MemoryGBstay at 0 (theomitemptyJSON tags hide them from API payloads), the rest ofDetailsis populated from the recommendation payload as before, and aWARNis logged once per client lifetime. Matches the contract from cache/cosmosdb/database in PR #81.Out of scope
Platform/Tenancy/Scopeenrichment — those need additional Azure data sources beyondarmcompute.ResourceSKU.Capabilities.common.ComputeDetails(fields already added in PR perf(common): extend ComputeDetails with VCPU + MemoryGB #97).Test plan
go build ./...go test github.com/LeanerCloud/CUDly/providers/azure/services/compute— 42 passed (4 new + all existing)go test github.com/LeanerCloud/CUDly/providers/azure/...— 309 passed across 10 packages (the pre-existingproviders/azure/services/searchbuild failure is unrelated —go.sumdrift)go test ./...frompkg/— 319 passedgo vet ./...— cleangofmt -l .— emptygocyclo -over 10on the modified file — no offendersSummary by CodeRabbit