Repository navigation
fix: bound instance type cache retention - #1847
Conversation
Flush derived instance type entries when quota, unavailable offerings, or SKU data changes instead of retaining obsolete generation-keyed graphs. Keep quota fail-open reset and recovery visible to cache invalidation, and cover cache cardinality across all invalidation sources.
Use atomic.Uint64 for quota and unavailable-offering generations, and encapsulate unavailable-offering reads behind SeqNum().
There was a problem hiding this comment.
Pull request overview
This PR addresses excessive memory retention in the Azure instance type cache by removing monotonically increasing source sequence numbers from cache keys and instead invalidating cached derived instance types when their underlying source generations change (quota, unavailable offerings, or SKU discovery). This aligns with the provider’s operational needs by preventing “unreachable cache key” accumulation while keeping legitimate NodeClass-parameter variants cacheable under the existing TTL.
Changes:
- Reworked instance type cache keying to be based only on NodeClass-derived construction parameters, and added whole-cache invalidation on quota/unavailable-offering generation changes.
- Updated quota and unavailable-offerings to use typed atomic counters and added reset/recovery invalidation semantics for quota.
- Added/extended tests to assert bounded cache cardinality and correct invalidation behavior across quota, unavailable-offering, and SKU refresh scenarios.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/providers/quota/quota.go | Switches quota seq counter to atomic.Uint64 and adds reset/recovery invalidation semantics. |
| pkg/providers/quota/quota_test.go | Adds coverage ensuring quota reset invalidates cached quota state and recovery triggers a new generation. |
| pkg/providers/instancetype/instancetypes.go | Replaces cache key scheme to avoid monotonically increasing key components; serializes rebuilds and invalidates cache on source-generation changes. |
| pkg/cache/unavailableofferings.go | Converts unavailable-offerings sequencing to typed atomic and exposes a SeqNum() accessor used by instance type cache generation tracking. |
| pkg/providers/instancetype/suite_test.go | Adds assertions around instance type cache cardinality and invalidation behavior for quota/offering/SKU changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Advance the unavailable-offering generation only when a mark introduces or strengthens a restriction, while still refreshing TTLs. Serialize compound mark updates and cover duplicate, stricter, global spot, and concurrent mark behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pkg/providers/instancetype/suite_test.go:2509
- Severity: Medium; Category: Test Coverage — InstanceTypeCache is shared across the whole suite (azureEnv is initialized once in TestAzure), so asserting an exact global ItemCount here is order-dependent and can flake if earlier specs populated additional cache keys. This check only needs to assert the cache is non-empty after the BeforeEach List().
Expect(azureEnv.InstanceTypeCache.ItemCount()).To(Equal(1))
pkg/providers/instancetype/suite_test.go:3344
- Severity: Medium; Category: Test Coverage — InstanceTypeCache is shared across the suite, so expecting it to be empty at the start of this spec is order-dependent. Flush the cache at the start of this test to ensure the cardinality assertions reflect only this scenario.
Expect(azureEnv.InstanceTypeCache.ItemCount()).To(BeZero())
pkg/providers/instancetype/suite_test.go:2277
- Severity: Medium; Category: Test Coverage — This suite uses a shared, package-level azureEnv, so InstanceTypeCache state can leak across specs. These assertions compare total cache cardinality and can become order-dependent (e.g., if earlier specs populated additional keys, UnavailableOfferingsCache.Flush will now flush/rebuild and the ItemCount equality can fail for reasons unrelated to the behavior under test). Flush the InstanceTypeCache at the start of the table entry so the cardinality assertions only reflect this scenario.
This issue also appears in the following locations of the same file:
- line 2509
- line 3344
Expect(cacheEntries).To(BeNumerically(">", 0))
// capacity shortage is over - expire the items from the cache and try again
azureEnv.UnavailableOfferingsCache.Flush()
ExpectProvisionedAndWaitForPromises(ctx, env.Client, cluster, cloudProvider, coreProvisioner, azureEnv, pod)
Expect(azureEnv.InstanceTypeCache.ItemCount()).To(Equal(cacheEntries))
Return generation-raced instance type snapshots uncached instead of retrying indefinitely while holding the cache mutex. Add deterministic coverage for quota availability changing during construction.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
pkg/providers/instancetype/instancetypes.go:188
- Severity: Medium | Category: Correctness
result is stored in instanceTypesCache and then returned directly on the cache-miss path. If any caller mutates the returned slice (e.g., reorders it), it will also mutate the cached value, which contradicts the shallow-copy protection you apply on cache hits and can lead to non-deterministic behavior across callers.
Consider caching a shallow copy and also returning a shallow copy when populating the cache, so callers can safely reorder without affecting future cache reads.
p.instanceTypesCache.SetDefault(key, result)
if latest := p.currentInstanceTypesCacheGeneration(); generation != latest {
Provider.List() cached `result` via SetDefault and then returned the same slice to the caller on the miss path, unlike the hit path which defensively copies. A caller reordering its returned slice (e.g. sort.Slice) mutated the shared backing array, silently corrupting the order seen by later cache hits. Return a shallow copy on the miss path too, matching the cache-hit behavior. Added a regression test that reorders the returned slice and asserts a subsequent List() call for the same key is unaffected.
Co-authored-by: Matthew Christopher
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
pkg/cache/unavailableofferings.go:58
- Spelling: comment says "offerrings"; should be "offerings".
// seqNum is updated on any material changes to unavailable offerrings cache (not updated on TTL only changes)
seqNum atomic.Uint64
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
pkg/cache/unavailableofferings.go:57
- Spelling: comment says "offerrings"; should be "offerings".
// seqNum is updated on any material changes to unavailable offerrings cache (not updated on TTL only changes)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
pkg/cache/unavailableofferings.go:58
- Spelling: comment typo in “unavailable offerrings cache”.
// seqNum is updated on any material changes to unavailable offerrings cache (not updated on TTL only changes)
seqNum atomic.Uint64
pkg/providers/instancetype/instancetypes.go:180
- Low (Correctness):
List()returns a shallow-copied slice on cache hits and normal cache misses, but the generation-race early-return path returns the rawresultslice. This makes the function’s return contract inconsistent and means a caller reordering the returned slice on this path behaves differently than on the other paths. Return a shallow copy here as well for consistency.
return result, nil
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
pkg/providers/instancetype/instancetypes.go:179
- Severity: Medium | Category: Correctness
This caches the instance type slice even if the quota/unavailable-offerings generation changes while buildInstanceTypes is running. That contradicts the PR description (“generation-raced snapshots uncached”), and can transiently retain/serve a cache entry built from mixed source data.
Consider re-checking the source-data generation after the build, and if it changed, avoid caching (and optionally flush/update the tracked generation) while still returning the one computed snapshot.
result := p.buildInstanceTypes(ctx, instanceTypeParams)
p.instanceTypesCache.SetDefault(key, result)
// Return a shallow copy, matching the cache-hit path, so a caller reordering its slice doesn't reorder the cached one.
return append([]*cloudprovider.InstanceType{}, result...), nil
pkg/cache/unavailableofferings.go:57
- Severity: Low | Category: Maintainability
Typo in comment: “offerrings” → “offerings”. This comment is used to document seqNum semantics, so fixing the spelling helps avoid confusion when grepping/searching.
// seqNum is updated on any material changes to unavailable offerrings cache (not updated on TTL only changes)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
pkg/providers/instancetype/instancetypes.go:179
- [Severity: Medium] [Category: Performance]
List()holdsmuInstanceTypesCachefor the entire cache-miss path, including the potentially expensivebuildInstanceTypes()loop (pricing/offerings/quota checks). This serializes all concurrentList()calls (even cache hits and different parameter keys) behind one miss, which can increase provisioning latency during bursts of scheduling activity.
Consider narrowing the critical section so only the generation check + cache get/set are locked (or using an RW lock / per-key dedupe) while building happens outside the global mutex.
p.muInstanceTypesCache.Lock()
defer p.muInstanceTypesCache.Unlock()
generation := p.currentInstanceTypesSourceDataGeneration()
if generation != p.instanceTypesCacheGeneration {
p.instanceTypesCache.Flush()
p.instanceTypesCacheGeneration = generation
}
if item, ok := p.instanceTypesCache.Get(key); ok {
// Ensure what's returned from this function is a shallow-copy of the slice (not a deep-copy of the data itself)
// so that modifications to the ordering of the data don't affect the original
return append([]*cloudprovider.InstanceType{}, item.([]*cloudprovider.InstanceType)...), nil
}
result := p.buildInstanceTypes(ctx, instanceTypeParams)
p.instanceTypesCache.SetDefault(key, result)
// Return a shallow copy, matching the cache-hit path, so a caller reordering its slice doesn't reorder the cached one.
return append([]*cloudprovider.InstanceType{}, result...), nil
Fixes #1844
Description
The fully initialized instance type cache currently includes monotonically increasing SKU, unavailable-offering, and quota sequence numbers in each cache key. Every availability change therefore retains another complete instance type graph until the 23-hour TTL expires.
This change:
The existing TTL remains responsible for removing unused NodeClass parameter variants rather than obsolete source-data generations.
How was this change tested?
go test ./pkg/cache ./pkg/providers/quota ./pkg/providers/instancetype -count=1go test -race ./pkg/cache ./pkg/providers/quota -count=1make verifyDoes this change impact docs?
Release Note