Repository navigation
feat(azure): org-wide multi-subscription recommendation collection (closes #553) - #1520
Conversation
cristim
added
priority/p2
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAzure subscription discovery now uses a synchronized cache with invalidation and single-flight fetching. Recommendation client resolution supports pinned, single-subscription, and multi-subscription modes. Multi-subscription results are fetched concurrently and returned with typed partial-failure errors. Scheduler and command flows retain successful partial results. ChangesAzure subscription-aware recommendations
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AzureProvider
participant SubscriptionAPI
participant MultiSubscriptionRecommendationsClient
participant RecommendationClients
Caller->>AzureProvider: GetRecommendationsClient
AzureProvider->>SubscriptionAPI: Discover accessible subscriptions
SubscriptionAPI-->>AzureProvider: Subscription accounts
AzureProvider-->>Caller: Single- or multi-subscription client
Caller->>MultiSubscriptionRecommendationsClient: GetRecommendations
MultiSubscriptionRecommendationsClient->>RecommendationClients: Query subscriptions concurrently
RecommendationClients-->>MultiSubscriptionRecommendationsClient: Recommendations and errors
MultiSubscriptionRecommendationsClient-->>Caller: Merged recommendations and partial-failure status
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/azure/provider.go (1)
280-373: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid holding
accountsMuduring the ARM list call
fetchAccountsLockedcallspager.NextPage(ctx)under the write lock, so any slow or hung subscription lookup blocks every concurrent caller that reaches account discovery. Add an internal timeout here, or move the fetch outside the mutex and only lock around cache population.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider.go` around lines 280 - 373, Update getOrFetchAccounts and fetchAccountsLocked so the ARM subscription listing does not run while accountsMu is held. Fetch accounts outside the mutex, then reacquire the lock to re-check cachedAccounts and populate it only if still empty, returning the existing cached result otherwise; preserve cloneAccounts behavior and propagate fetch errors unchanged.
🧹 Nitpick comments (1)
providers/azure/provider.go (1)
99-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the subscription-cache logic into its own file.
getOrFetchAccounts,cloneAccounts,InvalidateAccountsCache, andfetchAccountsLockedform a cohesive, self-contained unit (~100 lines) added to an already large file (content in this diff runs past line 635). As per coding guidelines, Go/TS files should be kept under 500 lines; extracting this caching subsystem into a sibling file (e.g.providers/azure/accounts_cache.go) would restore compliance with low mechanical risk and improve cohesion.Also applies to: 270-373
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider.go` around lines 99 - 109, The subscription-cache subsystem should be moved out of the large provider file into a sibling Go file. Extract getOrFetchAccounts, cloneAccounts, InvalidateAccountsCache, fetchAccountsLocked, and the accountsMu/cachedAccounts state together, preserving their behavior and existing access from GetAccounts, GetServiceClient, and GetRecommendationsClient.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@providers/azure/provider.go`:
- Around line 280-373: Update getOrFetchAccounts and fetchAccountsLocked so the
ARM subscription listing does not run while accountsMu is held. Fetch accounts
outside the mutex, then reacquire the lock to re-check cachedAccounts and
populate it only if still empty, returning the existing cached result otherwise;
preserve cloneAccounts behavior and propagate fetch errors unchanged.
---
Nitpick comments:
In `@providers/azure/provider.go`:
- Around line 99-109: The subscription-cache subsystem should be moved out of
the large provider file into a sibling Go file. Extract getOrFetchAccounts,
cloneAccounts, InvalidateAccountsCache, fetchAccountsLocked, and the
accountsMu/cachedAccounts state together, preserving their behavior and existing
access from GetAccounts, GetServiceClient, and GetRecommendationsClient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9598f7d5-1fb8-4c30-8e08-d7e1d1739c53
📒 Files selected for processing (4)
providers/azure/provider.goproviders/azure/provider_test.goproviders/azure/recommendations_multi_subscription.goproviders/azure/recommendations_multi_subscription_test.go
6a8155f to
bdd3260
Compare
|
@coderabbitai full review Force-pushed a fix for the concurrency finding on |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 35 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
providers/azure/provider_test.go (1)
1398-1407: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
runtime.Gosched()nudge doesn't guarantee the followers are on the cold-cache path.Nothing blocks until the followers actually reach
getOrFetchAccounts, soclose(gated.release)can fire while some goroutines haven't started. Those late callers then hit an already-warm cache, and a naive per-caller-fetch implementation would still count 1 — the regression this test exists to catch can slip through. Gating each follower on an explicit "entered" signal (e.g. a second counter/channel incremented beforeGetAccounts, plus a barrier) makes it deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider_test.go` around lines 1398 - 1407, Replace the runtime.Gosched-based coordination in the concurrent GetAccounts test with an explicit follower-entry barrier: have each follower signal an entered counter/channel immediately before invoking GetAccounts, wait until all followers have entered the cold-cache path, then close gated.release. Keep the existing worker synchronization and assertions unchanged so late goroutines cannot observe a warmed cache.providers/azure/accounts_cache.go (1)
43-68: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDetach the shared cache fill from the caller’s context.
getOrFetchAccountspasses the singleflight leader’sctxintofetchAccounts, so one canceled request can abort the shared ARM list call for every waiter and leave the cache cold. Usecontext.WithoutCancel(ctx)here, or retry once oncontext.Canceled.providers/azure/accounts_cache.go:43-68🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/accounts_cache.go` around lines 43 - 68, Update the shared fetch in getOrFetchAccounts so fetchAccounts receives a context detached from caller cancellation, using context.WithoutCancel(ctx). Keep the singleflight cache population and error propagation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/azure/provider.go`:
- Around line 457-470: Update the recommendation client resolution around
p.subscriptionID and getOrFetchAccounts to honor the subscription ID resolved by
the provider’s default-subscription logic, including AZURE_SUBSCRIPTION_ID,
before falling back to all visible accounts. Reuse the existing
resolveSubscriptionIDFromCtx or equivalent resolved-default path so
recommendations remain scoped consistently with GetServiceClient and GetRegions,
while preserving explicit p.subscriptionID handling and multi-account fan-out
only when no default is resolved.
In `@providers/azure/recommendations_multi_subscription_test.go`:
- Around line 43-47: Update the doc comment for withFakeSubscriptionClients to
state that it provides fake clients by subscriptionID from the supplied map,
rather than describing ordered allocation by account order. Keep the
cleanup/restoration behavior documentation accurate.
---
Nitpick comments:
In `@providers/azure/accounts_cache.go`:
- Around line 43-68: Update the shared fetch in getOrFetchAccounts so
fetchAccounts receives a context detached from caller cancellation, using
context.WithoutCancel(ctx). Keep the singleflight cache population and error
propagation unchanged.
In `@providers/azure/provider_test.go`:
- Around line 1398-1407: Replace the runtime.Gosched-based coordination in the
concurrent GetAccounts test with an explicit follower-entry barrier: have each
follower signal an entered counter/channel immediately before invoking
GetAccounts, wait until all followers have entered the cold-cache path, then
close gated.release. Keep the existing worker synchronization and assertions
unchanged so late goroutines cannot observe a warmed cache.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94f3473b-66c2-4c25-83fe-3c5fefbdfe43
📒 Files selected for processing (5)
providers/azure/accounts_cache.goproviders/azure/provider.goproviders/azure/provider_test.goproviders/azure/recommendations_multi_subscription.goproviders/azure/recommendations_multi_subscription_test.go
Adversarial review pass: 6 confirmed findings, all fixedReviewed with a focus on cross-subscription data bleed, since this PR is the first Azure path that can return more than one subscription's data from a single client. Six confirmed issues, each with a regression test verified to fail with its fix reverted and pass with it in place. Commits: 20d3e53 (azure), 671ced2 (scheduler). 1.
|
|
🐇🔍 ✏️ Learnings added
✅ Action performedReview finished.
|
31b45cb to
3e07ed2
Compare
|
Rebased onto History was rewritten, so this is a force-push: Since every commit SHA changed, the previous review no longer corresponds to anything on this branch. @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 39 minutes. |
|
Status update: head is The two nitpicks from the 16:17Z review were both addressed in The full review requested at 18:46Z was acknowledged but no review has landed since 16:17Z, so re-requesting. Please do a full pass, not an incremental one, since the branch was rebased after that review. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
providers/azure/recommendations_multi_subscription.go (1)
189-202: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffPartial failures are only visible in logs.
A sweep where 9 of 10 subscriptions fail returns
nilerror, so downstream aggregation/reporting treats the result as complete org-wide coverage. Consider returning the covered/failed subscription counts (or a partial-error type likeinternal/purchase'smultiAccountPartialError) so callers can annotate reports as incomplete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/recommendations_multi_subscription.go` around lines 189 - 202, Update the multi-subscription recommendation aggregation around the errs loop and its failures check so any mix of successful and failed subscriptions returns an explicit partial-failure signal, not a nil error. Include covered and failed subscription counts, or reuse the existing multiAccountPartialError pattern from internal/purchase, while preserving the all-failed error behavior and collected successful results.providers/azure/provider_test.go (1)
1614-1624: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCoverage gap: env target invisible with exactly one visible subscription.
This subtest pins the two-subscription case, where no default resolves and the fail-loud branch fires. The one-visible-subscription variant takes the opposite path (
resolveDefaultSubscriptionmarks the lone sub default, so Step 1 returns a client for a subscription the caller never named). Adding that case would pin the behavior either way — see the related comment onproviders/azure/provider.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider_test.go` around lines 1614 - 1624, Add a subtest alongside the existing invisible-subscription test that sets AZURE_SUBSCRIPTION_ID to an unseen value while configuring exactly one visible subscription, then call GetRecommendationsClient and assert the intended behavior from resolveDefaultSubscription: it must not return a client for the unnamed visible subscription, and should preserve the explicit-target error contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/azure/provider.go`:
- Around line 504-520: The subscription target validation occurs too late
because resolveDefaultSubscription can select the sole visible subscription
first. In providers/azure/provider.go lines 504-520, validate
AZURE_SUBSCRIPTION_ID against accounts before calling getDefaultSubscriptionID
and return the existing misconfiguration error when it is invisible; in
providers/azure/provider_test.go lines 1614-1624, add coverage for an invisible
target with exactly one visible subscription and assert the error outcome.
---
Nitpick comments:
In `@providers/azure/provider_test.go`:
- Around line 1614-1624: Add a subtest alongside the existing
invisible-subscription test that sets AZURE_SUBSCRIPTION_ID to an unseen value
while configuring exactly one visible subscription, then call
GetRecommendationsClient and assert the intended behavior from
resolveDefaultSubscription: it must not return a client for the unnamed visible
subscription, and should preserve the explicit-target error contract.
In `@providers/azure/recommendations_multi_subscription.go`:
- Around line 189-202: Update the multi-subscription recommendation aggregation
around the errs loop and its failures check so any mix of successful and failed
subscriptions returns an explicit partial-failure signal, not a nil error.
Include covered and failed subscription counts, or reuse the existing
multiAccountPartialError pattern from internal/purchase, while preserving the
all-failed error behavior and collected successful results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bc56d4d-e406-434b-a35f-2a232a3bfe7a
📒 Files selected for processing (7)
internal/scheduler/scheduler.gointernal/scheduler/scheduler_test.goproviders/azure/accounts_cache.goproviders/azure/provider.goproviders/azure/provider_test.goproviders/azure/recommendations_multi_subscription.goproviders/azure/recommendations_multi_subscription_test.go
CodeRabbit's Major finding: confirmed and fixed in
|
Response to the review on
|
Pre-merge check for any partial-failure change to
|
| Line | Path |
|---|---|
| 689 | Azure org-wide GetAllRecommendations |
| 708 | GCP org-wide GetAllRecommendations |
| 801 | Azure per-account GetAllRecommendations |
| 869 | GCP per-account GetAllRecommendations |
| 896 | generic per-provider GetAllRecommendations |
| 911 | generic default term/payment fallback GetRecommendations |
Lines 689, 801, and the 896/911 generic path are the routes that actually reach
the Azure multi-subscription client, so they are not hypothetical.
cmd/multi_service_helpers.go:441 is already handled in the in-flight work via
asPartialSubscriptionFailure, but that call site alone is not sufficient
coverage. The scheduler is the path that runs unattended, which makes it the
one where silently dropped data is least likely to be noticed.
Suggested verification before merge
- Every call site above either inspects the error with
errors.Asand keeps the
partial results, or is deliberately left failing closed with a stated reason. - A regression test on the scheduler path: a sweep where a strict subset of
subscriptions fails still records the successful subscriptions'
recommendations, rather than returning zero rows.
Also worth confirming the AWS and GCP recommendation clients, which implement
the same interface, are unaffected by the new error semantics.
Partial-failure signal added (
|
Second-order regression from the partial-failure error, fixed (
|
|
Requesting a full review (not incremental) now that a window may be open. Triggers are serialised centrally across the queue because CodeRabbit is under an account-level adaptive throttle and parallel pings hold it in backoff. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
providers/azure/recommendations_multi_subscription.go (2)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate package doc comment.
providers/azure/provider.goalready carries thePackage azuredoc; a second one in this file means tooling picks an arbitrary winner. Demote it to a plain file comment (// Org-wide (multi-subscription) recommendations fan-out client.).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/recommendations_multi_subscription.go` around lines 1 - 3, Replace the package doc comment in the multi-subscription recommendations file with a plain file comment, leaving the existing Package azure documentation in provider.go as the sole package-level doc.
79-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Error()panics on an emptyFailedslice.
e.Failed[0].Erris unguarded.mergeSubscriptionResultsnever produces an emptyFailed, but the type and its fields are exported, so a zero-value or externally constructed instance panics the moment it is formatted — typically inside a log/error-wrapping path.🛡️ Guard the first-cause read
func (e *PartialSubscriptionFailureError) Error() string { + if len(e.Failed) == 0 { + return fmt.Sprintf("azure recommendations incomplete: %d of %d subscriptions succeeded", + e.Succeeded, e.Attempted) + } return fmt.Sprintf( "azure recommendations incomplete: %d of %d subscriptions succeeded; %d failed (%s): %v", e.Succeeded, e.Attempted, len(e.Failed), strings.Join(e.FailedSubscriptionIDs(), ", "), e.Failed[0].Err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/recommendations_multi_subscription.go` around lines 79 - 84, Update PartialSubscriptionFailureError.Error to guard access to e.Failed[0].Err when Failed is empty. Preserve the existing first-failure detail for non-empty slices, while returning a safe error message for zero-value or externally constructed instances without panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/azure/provider.go`:
- Around line 169-197: Synchronize credential and subscriptions-client swaps
with the accounts cache mutex to eliminate races and mixed snapshots. Update
SetCredential and SetSubscriptionsClient to assign their fields while holding
accountsMu, and update fetchAccounts to snapshot cred and subscriptionsClient
under RLock before using them; retain cache invalidation after the synchronized
publication.
---
Nitpick comments:
In `@providers/azure/recommendations_multi_subscription.go`:
- Around line 1-3: Replace the package doc comment in the multi-subscription
recommendations file with a plain file comment, leaving the existing Package
azure documentation in provider.go as the sole package-level doc.
- Around line 79-84: Update PartialSubscriptionFailureError.Error to guard
access to e.Failed[0].Err when Failed is empty. Preserve the existing
first-failure detail for non-empty slices, while returning a safe error message
for zero-value or externally constructed instances without panicking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70fdd710-c9da-42f4-9388-8eedd2dcbff2
📒 Files selected for processing (8)
cmd/multi_service_helpers.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_test.goproviders/azure/accounts_cache.goproviders/azure/provider.goproviders/azure/provider_test.goproviders/azure/recommendations_multi_subscription.goproviders/azure/recommendations_multi_subscription_test.go
|
Pushed Fixed1. A partially-satisfied
2. 3. Unsynchronized Both behavioral fixes carry regression tests confirmed to fail on the pre-fix code, not merely to pass on the fixed code:
Disclosed (not "fixed" — the code stays as scaffolding)The PR body now states plainly that this diff is a behavioral no-op in production today, naming all three unreachable integration surfaces rather than just the first: the fan-out client itself, the Azure partial-handling in I verified each claim against the code rather than taking the review's word for it — including the one place the reviewer's summary was imprecise: Not deleting the code: it is scaffolding for the follow-up that wires it up. But it should not read as shipped either. Deferred (filed, not fixed here)
GatesAll run locally, exit codes reported rather than inferred from empty output:
The golangci run used the exact CI invocation (repo root, no path arguments) at the version @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff stat ==='
git diff --stat origin/main...HEAD || git diff --stat HEAD~1..HEAD
echo '=== Changed files ==='
git diff --name-only origin/main...HEAD || git diff --name-only HEAD~1..HEAD
echo '=== Azure symbol outlines ==='
for f in \
providers/azure/recommendations_multi_subscription.go \
providers/azure/accounts_cache.go \
providers/azure/provider.go \
internal/scheduler/scheduler.go \
cmd/multi_service_helpers.go \
internal/purchase/execution.go
do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all 2>/dev/null || true
fi
done
echo '=== Recommendation-client construction and calls ==='
rg -n -C 3 --glob '*.go' \
'(NewAzureProvider|GetRecommendationsClient\s*\(|GetAllRecommendations\s*\(|GetRecommendations\s*\(|tolerateIncompleteSweep|AsPartialSubscriptionFailure|selectSubscriptions|ErrSubscriptionNotAccessible|SetCredential\s*\(|SetSubscriptionsClient\s*\()' \
providers internal cmd pkgLength of output: 173995 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Multi-subscription implementation ==='
sed -n '35,395p' providers/azure/recommendations_multi_subscription.go
echo '=== Account cache implementation ==='
sed -n '40,305p' providers/azure/accounts_cache.go
echo '=== Azure provider synchronization and client resolution ==='
sed -n '90,255p' providers/azure/provider.go
sed -n '525,615p' providers/azure/provider.go
echo '=== Scheduler partial-error policy and Azure construction ==='
sed -n '670,825p' internal/scheduler/scheduler.go
sed -n '890,965p' internal/scheduler/scheduler.go
echo '=== Regression tests for new filter/error behavior ==='
sed -n '135,455p' providers/azure/recommendations_multi_subscription_test.go
echo '=== Regression tests for concurrency / credential swaps ==='
sed -n '1360,1675p' providers/azure/provider_test.go
echo '=== Full diff, compact context ==='
git diff --unified=20 origin/main...HEAD -- \
providers/azure/recommendations_multi_subscription.go \
providers/azure/accounts_cache.go \
providers/azure/provider.go \
internal/scheduler/scheduler.go \
cmd/multi_service_helpers.go \
| sed -n '1,2200p'Length of output: 149404 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== All AzureProvider mutable-field accesses ==='
rg -n -C 4 --glob '*.go' \
'p\.(cred|credErr|credOnce|credProvider|subscriptionsClient|subscriptionID|cachedAccounts|accountsGen)\b' \
providers/azure
echo '=== IsConfigured and initialization tests ==='
sed -n '250,360p' providers/azure/provider.go
sed -n '115,245p' providers/azure/provider_test.go
rg -n -C 5 --glob '*test.go' \
'(IsConfigured|SetCredentialProvider|SetCredential).*([Cc]oncurrent|[Rr]ace)|([Cc]oncurrent|[Rr]ace).*(IsConfigured|SetCredentialProvider|SetCredential)' \
providers/azure
echo '=== PR body and current head ==='
git rev-parse --short HEAD
gh pr view 1520 --repo LeanerCloud/CUDly --json title,body,headRefOid,mergeStateStatus,statusCheckRollup \
--jq '{title, headRefOid, mergeStateStatus, checks: [.statusCheckRollup[] | {name, conclusion: .conclusion}], body}'Length of output: 26961
One issue remains:
I did not identify additional blockers in the reported AccountFilter or cache fixes. 🐇🔍 ✏️ Learnings added
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/azure/provider.go (1)
272-293: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
IsConfiguredcan overwrite a credential installed by a concurrentSetCredential.Line 274 reads the credential, and line 290 publishes a lazily created one. The two steps are separate critical sections. If
SetCredentiallands between them,publishCredentialreplaces the injected credential with the ambient one.publishCredentialalso skips cache invalidation, so the accounts cache then belongs to a credential that is no longer installed.
SetCredentialis a production path (scheduler and purchase execution install per-account federated credentials), so this is the same class of hazard the rest of this PR closed.Make the publish conditional on the field still being nil under one lock.
🔒 Publish only when no credential was installed meanwhile
-// publishCredential installs cred under accountsMu without touching the -// accounts cache. +// publishCredentialIfUnset installs cred under accountsMu only when no +// credential is installed, and reports whether it published. // // Used by IsConfigured's lazy ambient-credential resolution, which only runs // when no credential was installed at all. Unlike SetCredential it does not // invalidate: there is no previous credential whose subscription list could -// have been cached under it. -func (p *AzureProvider) publishCredential(cred azcore.TokenCredential) { +// have been cached under it. The nil re-check under the write lock closes the +// window in which a concurrent SetCredential would otherwise be overwritten. +func (p *AzureProvider) publishCredentialIfUnset(cred azcore.TokenCredential) bool { p.accountsMu.Lock() defer p.accountsMu.Unlock() + if p.cred != nil { + return false + } p.cred = cred + return true }- p.publishCredential(cred) + p.publishCredentialIfUnset(cred)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider.go` around lines 272 - 293, Update AzureProvider.IsConfigured and the credential publication path so the lazily created credential is assigned only while holding the same lock that guards the credential field, and only if the field remains nil. Preserve any credential installed concurrently by SetCredential, including its cache state, instead of unconditionally calling publishCredential after credential creation.
♻️ Duplicate comments (1)
providers/azure/recommendations_multi_subscription_test.go (1)
56-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe doc comment still contradicts the implementation.
withFakeSubscriptionClientsresolves each fake bysubscriptionIDfrom thefakesmap and returns an error for an unregistered ID. It does not hand out fakes in account order. A previous review raised this and the thread is marked addressed, but the comment text is unchanged in this revision.📝 Reword the doc comment
// withFakeSubscriptionClients overrides newSubscriptionRecommendationsClientFn -// to hand out the given fakes in order (one per NewMultiSubscriptionRecommendationsClient -// account, in the order accounts are passed) and restores the original on -// cleanup. +// to return the fake registered for each requested subscription ID, erroring +// on an unregistered ID, and restores the original factory on cleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/recommendations_multi_subscription_test.go` around lines 56 - 59, Update the doc comment for withFakeSubscriptionClients to describe that it resolves fake subscription clients by subscriptionID from the fakes map and returns an error when an ID is unregistered, rather than handing them out in account order.
🧹 Nitpick comments (5)
cmd/multi_service_helpers.go (1)
103-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit these oversized Go files.
Each final file exceeds the 500-line limit. Split coherent helpers and test groups into necessary non-root files. Keep each resulting Go file below 500 lines.
cmd/multi_service_helpers.go#L103-L109: move region-discovery and recommendation-fetch helpers into focused command helper files.internal/scheduler/scheduler.go#L894-L935: move recommendation collection or partial-sweep handling into focused scheduler files.internal/scheduler/scheduler_test.go#L707-L804: split scheduler tests by collection domain.As per coding guidelines, “keep files under 500 lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/multi_service_helpers.go` around lines 103 - 109, Split the oversized files into focused non-root files while preserving behavior: in cmd/multi_service_helpers.go, move region-discovery and recommendation-fetch helpers; in internal/scheduler/scheduler.go, move recommendation collection or partial-sweep handling; and in internal/scheduler/scheduler_test.go, split tests by collection domain. Keep every resulting Go file below 500 lines and update package-level references as needed.Source: Coding guidelines
providers/azure/provider_test.go (2)
1483-1504: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe swapper goroutines spin without a yield.
Both loops call
SetCredentialorSetSubscriptionsClientin a tight loop with no pause. Each call takes the write lock and bumps the generation, so the readers re-fetch on every round and the two swappers burn a core each untilstopcloses. Under-racethe instrumentation multiplies that cost.Add a
runtime.Gosched()at the end of each loop body. The race window stays open, and the test stops saturating two cores.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider_test.go` around lines 1483 - 1504, Update both swapper goroutines in the test to call runtime.Gosched() at the end of each loop iteration after SetCredential or SetSubscriptionsClient, preserving the existing stop checks while yielding execution and avoiding tight-loop CPU saturation.
1385-1386: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the inherited-cancellation retry.
getOrFetchAccountsinproviders/azure/accounts_cache.go(Lines 54-68) retries once when the singleflight leader returns a context error and the caller's own context is still live. No test in this file drives that branch. The branch decides whether an unrelated caller's cancellation aborts a healthy caller, so a regression there would be silent.A gated subscriptions client whose first call returns
context.Canceledand whose second call returns the fixture list would cover it. I can generate the test if you want.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider_test.go` around lines 1385 - 1386, Add a test in TestAzureProvider_GetAccounts_ConcurrentColdCache_SingleARMCall covering getOrFetchAccounts’ inherited-cancellation retry: use a gated subscriptions client whose first request returns context.Canceled and whose second returns the fixture accounts, then verify a caller with a live context succeeds and the fixture is returned after the retry.providers/azure/provider.go (2)
92-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
provider.gonow exceeds the 500-line file limit.The coding guidelines require Go files under 500 lines. This file runs past 610 lines after the change. The cache subsystem was already split into
providers/azure/accounts_cache.go. Consider moving the recommendation-client selection logic (Lines 507-612) into a separate file in the same package, for exampleproviders/azure/recommendations_client_selection.go.As per coding guidelines: "Follow Domain-Driven Design with bounded contexts, keep files under 500 lines, and use typed interfaces for public APIs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider.go` around lines 92 - 126, Reduce provider.go below the 500-line limit by moving the recommendation-client selection logic from the recommendation client methods into a new same-package file such as recommendations_client_selection.go. Preserve the existing AzureProvider behavior and method APIs, moving only the relevant selection implementation while retaining access to the provider’s existing fields and helpers.Source: Coding guidelines
564-591: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnv-variable resolution is duplicated between two files.
resolveDefaultSubscriptioninproviders/azure/accounts_cache.go(Lines 247-273) already readsazureSubscriptionIDEnvand applies the priority rules. Step 1 here reads the same variable again and re-implements the "target must be visible" rule thatresolveDefaultSubscriptiondeliberately does not enforce. The two rule sets must stay consistent by hand.Consider moving the visibility check into a single helper in
accounts_cache.go, for exampleresolveScopedSubscription(accounts, explicitSubID) (string, error), and calling it from both sites. This is optional; the current behavior is correct and well covered by tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider.go` around lines 564 - 591, Centralize explicit subscription resolution and visibility validation instead of duplicating it in the provider flow. Add a helper near resolveDefaultSubscription in accounts_cache.go, such as resolveScopedSubscription, that accepts accounts and the explicit subscription ID, returns the validated ID or the existing error, and reuse it from both resolveDefaultSubscription and the Step 1 logic around getDefaultSubscriptionID in the provider. Preserve the current precedence and fail-loud behavior for configured IDs that are not visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/azure/accounts_cache.go`:
- Around line 257-273: Update resolveSubscriptionIDFromCtx to validate a
configured AZURE_SUBSCRIPTION_ID against visible subscriptions using the same
check as GetRecommendationsClient. If the target is not visible, return the
appropriate error instead of allowing resolveDefaultSubscription’s
single-subscription fallback; preserve fallback behavior only when no target is
configured.
In `@providers/azure/provider.go`:
- Around line 186-188: Synchronize the credProvider write in
SetCredentialProvider with accountsMu, matching the publication pattern used by
other swappable fields. Update IsConfigured to obtain the provider through the
existing credentialProvider() accessor inside credOnce.Do, preserving the
fallback to realCredentialProvider when no provider is configured.
---
Outside diff comments:
In `@providers/azure/provider.go`:
- Around line 272-293: Update AzureProvider.IsConfigured and the credential
publication path so the lazily created credential is assigned only while holding
the same lock that guards the credential field, and only if the field remains
nil. Preserve any credential installed concurrently by SetCredential, including
its cache state, instead of unconditionally calling publishCredential after
credential creation.
---
Duplicate comments:
In `@providers/azure/recommendations_multi_subscription_test.go`:
- Around line 56-59: Update the doc comment for withFakeSubscriptionClients to
describe that it resolves fake subscription clients by subscriptionID from the
fakes map and returns an error when an ID is unregistered, rather than handing
them out in account order.
---
Nitpick comments:
In `@cmd/multi_service_helpers.go`:
- Around line 103-109: Split the oversized files into focused non-root files
while preserving behavior: in cmd/multi_service_helpers.go, move
region-discovery and recommendation-fetch helpers; in
internal/scheduler/scheduler.go, move recommendation collection or partial-sweep
handling; and in internal/scheduler/scheduler_test.go, split tests by collection
domain. Keep every resulting Go file below 500 lines and update package-level
references as needed.
In `@providers/azure/provider_test.go`:
- Around line 1483-1504: Update both swapper goroutines in the test to call
runtime.Gosched() at the end of each loop iteration after SetCredential or
SetSubscriptionsClient, preserving the existing stop checks while yielding
execution and avoiding tight-loop CPU saturation.
- Around line 1385-1386: Add a test in
TestAzureProvider_GetAccounts_ConcurrentColdCache_SingleARMCall covering
getOrFetchAccounts’ inherited-cancellation retry: use a gated subscriptions
client whose first request returns context.Canceled and whose second returns the
fixture accounts, then verify a caller with a live context succeeds and the
fixture is returned after the retry.
In `@providers/azure/provider.go`:
- Around line 92-126: Reduce provider.go below the 500-line limit by moving the
recommendation-client selection logic from the recommendation client methods
into a new same-package file such as recommendations_client_selection.go.
Preserve the existing AzureProvider behavior and method APIs, moving only the
relevant selection implementation while retaining access to the provider’s
existing fields and helpers.
- Around line 564-591: Centralize explicit subscription resolution and
visibility validation instead of duplicating it in the provider flow. Add a
helper near resolveDefaultSubscription in accounts_cache.go, such as
resolveScopedSubscription, that accepts accounts and the explicit subscription
ID, returns the validated ID or the existing error, and reuse it from both
resolveDefaultSubscription and the Step 1 logic around getDefaultSubscriptionID
in the provider. Preserve the current precedence and fail-loud behavior for
configured IDs that are not visible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce53246c-c2dd-4654-b5d6-2771aa0a751f
📒 Files selected for processing (8)
cmd/multi_service_helpers.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_test.goproviders/azure/accounts_cache.goproviders/azure/provider.goproviders/azure/provider_test.goproviders/azure/recommendations_multi_subscription.goproviders/azure/recommendations_multi_subscription_test.go
Addresses both CodeRabbit findings from the review round on PR #1520. Both harden invariants that the previous commit established on one path and left off the others, which reads as a global guarantee but is not. 1. Silent subscription retarget (providers/azure/provider.go) resolveSubscriptionIDFromCtx -- which backs GetRegions and GetServiceClient -- called getDefaultSubscriptionID without first validating an explicitly configured subscription against the discovered list. resolveDefaultSubscription deliberately falls through to its single-visible-subscription rule when the configured target is not found, so with an unreachable AZURE_SUBSCRIPTION_ID (or ProviderConfig subscription) and exactly one visible subscription, the request was silently retargeted to whichever subscription happened to be visible. GetRecommendationsClient already guarded this via accountsContain under a comment reading "This ordering is load-bearing"; the same misconfiguration therefore errored on the recommendations path and silently retargeted on the regions/service path. GetServiceClient drives the resource enumeration recommendations are computed from, and is also the purchase-execution path's service-client lookup, so the operator could believe they were scoped to subscription A while data came from B. The guard is extracted into validateConfiguredSubscription and reused by GetRecommendationsClient, so the two paths cannot drift apart again. It covers both configuration axes (ProviderConfig field and the env var), mirroring resolveDefaultSubscription's own priority order. Production Azure flows pin AzureSubscriptionID (scheduler, registration and account handlers) and short-circuit before this path, so the guard engages only on the unpinned/ambient path where the retarget lived. GetRegions' error wrapper no longer claims "no Azure subscriptions found": the new inner error fires precisely when subscriptions were found but the configured one is not among them. 2. Unsynchronized credProvider (providers/azure/provider.go) SetCredentialProvider wrote p.credProvider outside accountsMu while IsConfigured read it inside credOnce.Do -- the one swappable field left outside the locking discipline the rest of the provider follows. Latent today (SetCredentialProvider has no production callers), but a half-kept invariant reads as a kept one. Published and read under accountsMu now, matching cred and subscriptionsClient. It deliberately does not invalidate the accounts cache: credProvider only feeds the lazy resolution that runs when no credential was installed at all, so no subscription list resolved under a different credential can exist yet. Regression tests, both confirmed failing against the pre-fix code: - TestAzureProvider_ResolveSubscription_InvisibleConfiguredTargetErrors uses an invisible configured target with exactly ONE visible subscription -- the case the fall-through hides. With two or more visible subscriptions getDefaultSubscriptionID returns "" and the pre-existing "multiple Azure subscriptions" error fires either way, so such a test would pass with or without the guard. Covers GetRegions and GetServiceClient, both configuration axes, and the honoured visible-target case. Pre-fix: 3 of 4 subtests fail with "An error is expected but got nil". - TestAzureProvider_ConcurrentCredentialProviderSwapAndIsConfigured_NoDataRace loops over fresh providers because credOnce makes the read one-shot per instance. Pre-fix: WARNING: DATA RACE, write at provider.go:187 against read at provider.go:280. TestAzureProvider_GetRegions and TestAzureProvider_GetServiceClient_WithSubscriptionLookup now clear AZURE_SUBSCRIPTION_ID via the existing helper: their fixtures resolve through the newly-guarded path, so an exported value on a developer machine would fail them for an unrelated reason.
30d5781 to
4bc85e5
Compare
Cache the ARM subscriptions list on AzureProvider (getOrFetchAccounts, double-checked locking, InvalidateAccountsCache for tests) so repeated GetAccounts/GetServiceClient/GetRecommendationsClient calls in one run do not each re-issue the subscriptions.List API call. Add MultiSubscriptionRecommendationsClient, which fans recommendation collection out across every subscription visible to the authenticated principal via errgroup, isolating per-subscription errors and only failing when every subscription fails. GetRecommendationsClient now returns this fan-out client when no subscription is pinned and 2+ are discovered, bringing Azure to parity with the AWS provider's automatic whole-organization recommendation coverage. Pinned-subscription and single-subscription paths are unchanged. Closes #553
… the ARM fetch getOrFetchAccounts held accountsMu.Lock() for the entire subscriptions.List network round-trip via fetchAccountsLocked, so every reader of the cached subscription list blocked behind a slow or hung ARM call. Move the fetch into golang.org/x/sync/singleflight so concurrent cold-cache callers coalesce into one in-flight request, taking accountsMu only to read or populate cachedAccounts. Add a deterministic regression test that stages N goroutines through a channel-gated fake fetch and asserts exactly one underlying ARM call.
Adversarial review of the org-wide fan-out. Scoping first. GetRecommendationsClient ignored the resolved default subscription. Before fan-out existed, an unpinned provider resolved the default via resolveSubscriptionIDFromCtx, which honours AZURE_SUBSCRIPTION_ID and errors when the scope is ambiguous. The fan-out path replaced that with "2+ visible subscriptions means fan out", so a caller that pinned a subscription through the environment silently started receiving every other visible subscription's recommendations. Restore the precedence: a resolvable default still yields a single-subscription client, a configured AZURE_SUBSCRIPTION_ID naming a subscription this principal cannot see is an error, and only a genuinely unnamed scope fans out. MultiSubscriptionRecommendationsClient ignored params.AccountFilter. The AWS provider applies that filter to every recommendation it returns (filterByAccounts in providers/aws/service_client.go). It was moot while an Azure client could only ever cover its own subscription, but a client covering every visible subscription has to honour it or a request scoped to one account comes back carrying others'. Filter before the fan-out so subscriptions outside the filter are never queried, and error rather than return an empty slice when the filter matches nothing. Then the subscription-list cache. InvalidateAccountsCache was a no-op against an in-flight fetch: a fetch that started before the invalidation still published its pre-invalidation snapshot afterwards, so the next read was served from cache and handed back exactly the data the caller asked to discard, with no second ARM call. A caller arriving in that window also joined the still in-flight pre-invalidation call through the constant singleflight key. Both are closed by an accountsGen counter that keys the singleflight call and gates the cache write. singleflight hands the leader's error to every waiter, so a caller whose own context was still live inherited a cancellation raised by whichever unrelated caller won the leader election. Retry once as leader when our own context is still healthy; our own cancellation stays terminal, so this cannot spin on a dead context. The unchecked type assertion on singleflight's untyped result now returns an error instead of panicking. Test fixes: the context-cancellation test passed with the guard it named deleted, because fakes that return ctx.Err() make every subscription fail and the all-failed guard produces the same error either way; its fakes now ignore the cancelled context so only the post-Wait ctx.Err() check can produce the error. The new cache tests read AZURE_SUBSCRIPTION_ID from the ambient environment and now neutralise it, the concurrent cold-cache test waits for every follower to enter GetAccounts rather than relying on a runtime.Gosched nudge, and the constructor test asserts the built client is actually stored. Each behaviour has a regression test verified to fail with its fix reverted. Refs #553
collectAzureForAccount tags every recommendation it returns with the registered account's UUID, so the provider has to be pinned to that account's subscription. It built the provider from acct.AzureSubscriptionID without checking it was set, and validateCloudAccountRequest does not require the field, so an azure row created without one produced an unpinned provider. That was harmless while an unpinned provider resolved a single subscription or errored. With the org-wide fan-out it instead collects from every subscription the credential can see, and those results are then filed under this one account, where anyone authorised for it can read them. Reject the misconfigured row by name instead.
SetCredential is used in production, not only in tests: the scheduler and purchase-execution paths construct the provider and then install per-account federated credentials. The subscription-list cache added in this PR records what the previous credential could see, so leaving it in place across a swap would report subscriptions the new principal may have no access to, and GetRecommendationsClient would fan out across them. Every caller today installs the credential before the first accounts fetch, so this is a guard against a future reordering rather than a live leak. SetSubscriptionsClient gets the same treatment for the same reason.
CodeRabbit finding. GetRecommendationsClient consulted getDefaultSubscriptionID before validating the configured target, and resolveDefaultSubscription's rule 3 marks a lone visible subscription as the default even when a target was configured and did not match it. So with AZURE_SUBSCRIPTION_ID naming a subscription the credential cannot see and exactly one subscription visible, that one subscription was returned as the default and the fail-loud check further down was unreachable. An operator who typo'd the variable, or whose credential lost access to the intended subscription, silently collected against the wrong subscription instead of getting an error: a misconfiguration answered with a plausible wrong result. Validate the target against the discovered accounts first, via a direct membership check rather than IsDefault, so it fails loud regardless of how many subscriptions are visible. A target that IS visible is honoured directly. The previous test only covered two visible subscriptions, where rule 3 never fires; the new subtest pins the single-visible-subscription case and fails against the old ordering.
mergeSubscriptionResults returned a nil error whenever at least one subscription succeeded, so 3 of 50 subscriptions failing handed back 47 subscriptions' recommendations indistinguishably from a complete sweep. The only signal was a log line, which no programmatic caller reads. On a path whose output is persisted and rendered as a savings opportunity, an under-collected sweep then reads as a shrinking opportunity rather than a failed one. Add PartialSubscriptionFailureError, returned ALONGSIDE the successful subscriptions' recommendations, carrying the attempted/succeeded counts and every failed subscription with its cause. Unwrap exposes the per-subscription causes so errors.Is still matches through it. Failing the whole sweep on one transient error would be worse than a partial result, so the data is still returned; callers opt in to keeping it by inspecting the error with errors.As, and a caller that treats any non-nil error as fatal now fails loud instead of silently under-reporting. Both outcomes beat the previous silent success. Updated the two call sites that can reach the fan-out (the CLI paths, which are the only ones that construct an unpinned Azure provider) to keep the partial data and report the gap. The scheduler's Azure paths always pin a subscription -- collectAzureAmbient via AZURE_SUBSCRIPTION_ID and collectAzureForAccount via the account's subscription -- so they never build the fan-out client and need no change. Tests assert the distinguishing property directly: a sweep where a subscription was never queried and a complete sweep that found nothing both produce an empty slice, and only the error tells them apart. Verified failing against the old nil-error contract.
Second-order effect of returning a typed partial-failure error. Every
recommendation-fetch site in the scheduler has the shape
recs, err := recClient.GetAllRecommendations(ctx)
if err != nil { return nil, ... }
which discards recs. Correct for a real error, but for a partial sweep it
turned one flaky subscription out of fifty into a total collection outage:
strictly worse than the silent under-collection the typed error exists to
prevent. The diff that introduced it looked like a pure improvement in
isolation and only misbehaved at callers that were not re-read.
Add tolerateIncompleteSweep and route every fetch site through it, so the
policy lives in one place instead of relying on each call site to remember
it. A partial sweep logs a warning naming the subscriptions that were not
queried and returns nil, so the data that was collected is kept and
persisted. Any other error is returned unchanged.
Also export AsPartialSubscriptionFailure from the azure provider so the
scheduler and the CLI share one definition of the check rather than each
hand-rolling errors.As, and add FailedSubscriptionIDs for the log lines.
Recording the incompleteness in the state table's last_collection_error, so
the dashboard shows the sweep as partial rather than merely smaller, needs a
partial-note threaded through collectProviderRecommendations and its three
per-provider implementations. Left as follow-up rather than bolted on here;
the warning names the failed subscriptions in the meantime.
Tests drive fetchAndConvert with a client returning recommendations plus a
partial-failure error and assert the data survives, that a genuine error
still fails loud, and that the helper is narrow. Verified failing with the
tolerance removed.
The Lint Code job pins golangci-lint v2.10.1 and .golangci.yml sets misspell locale to US, so the British spelling in these two comments failed the lint gate and cascaded to the CI Success aggregate: internal/scheduler/scheduler.go:910 internal/scheduler/scheduler_test.go:766 Both are comment prose, so this changes no identifier, no database column name and no string any test asserts on. Scoped deliberately to the two lines the linter flagged. 49 further occurrences of the British spelling remain elsewhere in the tree and were left alone: they are pre-existing, unrelated to this branch, and a repo-wide rewrite here would bury a two-word fix under a diff touching dozens of files.
Three review findings on the org-wide multi-subscription fan-out. 1. A partially-satisfied AccountFilter silently narrowed the sweep. A filter matching NO accessible subscription already errored, but one matching some and missing others returned the matched subscriptions' recommendations with a nil error. An operator scoping a sweep to sub-A and sub-B whose principal has lost Reader on sub-B therefore read sub-B as "no savings available" rather than "never queried" -- the exact confusion PartialSubscriptionFailureError exists to prevent, and which selectSubscriptions' own doc comment says must not happen. selectSubscriptions now iterates the filter rather than the discovered subscriptions, so every requested entry is accounted for as matched or missed, and the misses are folded into the partial-failure error as ErrSubscriptionNotAccessible. They count towards Attempted and Failed; the subscriptions that did answer still return their data, so one revoked subscription does not become a total collection outage. The filter is also deduplicated, so a repeated entry is neither queried twice nor double-counted. 2. PartialSubscriptionFailureError.Error() indexed Failed[0] unguarded. The type and its fields are exported, so a zero-value or externally-constructed instance panicked when formatted -- an error that crashes when logged fails exactly when the message is needed. 3. SetCredential and SetSubscriptionsClient wrote cred/subscriptionsClient as plain assignments and only then took accountsMu to invalidate, while fetchAccounts read both from a goroutine running on behalf of every caller that joined the single-flight. Both fields are now published inside one accountsMu critical section (via invalidateAccountsCacheLocked, since RWMutex is not reentrant) and snapshotted together under RLock, which also closes the window where a fetch pairs the old client with the new credential. The remaining in-package reads go through the same accessors so the guarantee holds rather than covering only one path. No production caller is concurrent today, so this was latent. Both behavioral fixes are covered by tests confirmed to fail on the pre-fix code: the partial-filter test with "An error is expected but got nil", and the swap test with a data race reported by -race. Refs #1652, #1653, #1654
Addresses both CodeRabbit findings from the review round on PR #1520. Both harden invariants that the previous commit established on one path and left off the others, which reads as a global guarantee but is not. 1. Silent subscription retarget (providers/azure/provider.go) resolveSubscriptionIDFromCtx -- which backs GetRegions and GetServiceClient -- called getDefaultSubscriptionID without first validating an explicitly configured subscription against the discovered list. resolveDefaultSubscription deliberately falls through to its single-visible-subscription rule when the configured target is not found, so with an unreachable AZURE_SUBSCRIPTION_ID (or ProviderConfig subscription) and exactly one visible subscription, the request was silently retargeted to whichever subscription happened to be visible. GetRecommendationsClient already guarded this via accountsContain under a comment reading "This ordering is load-bearing"; the same misconfiguration therefore errored on the recommendations path and silently retargeted on the regions/service path. GetServiceClient drives the resource enumeration recommendations are computed from, and is also the purchase-execution path's service-client lookup, so the operator could believe they were scoped to subscription A while data came from B. The guard is extracted into validateConfiguredSubscription and reused by GetRecommendationsClient, so the two paths cannot drift apart again. It covers both configuration axes (ProviderConfig field and the env var), mirroring resolveDefaultSubscription's own priority order. Production Azure flows pin AzureSubscriptionID (scheduler, registration and account handlers) and short-circuit before this path, so the guard engages only on the unpinned/ambient path where the retarget lived. GetRegions' error wrapper no longer claims "no Azure subscriptions found": the new inner error fires precisely when subscriptions were found but the configured one is not among them. 2. Unsynchronized credProvider (providers/azure/provider.go) SetCredentialProvider wrote p.credProvider outside accountsMu while IsConfigured read it inside credOnce.Do -- the one swappable field left outside the locking discipline the rest of the provider follows. Latent today (SetCredentialProvider has no production callers), but a half-kept invariant reads as a kept one. Published and read under accountsMu now, matching cred and subscriptionsClient. It deliberately does not invalidate the accounts cache: credProvider only feeds the lazy resolution that runs when no credential was installed at all, so no subscription list resolved under a different credential can exist yet. Regression tests, both confirmed failing against the pre-fix code: - TestAzureProvider_ResolveSubscription_InvisibleConfiguredTargetErrors uses an invisible configured target with exactly ONE visible subscription -- the case the fall-through hides. With two or more visible subscriptions getDefaultSubscriptionID returns "" and the pre-existing "multiple Azure subscriptions" error fires either way, so such a test would pass with or without the guard. Covers GetRegions and GetServiceClient, both configuration axes, and the honoured visible-target case. Pre-fix: 3 of 4 subtests fail with "An error is expected but got nil". - TestAzureProvider_ConcurrentCredentialProviderSwapAndIsConfigured_NoDataRace loops over fresh providers because credOnce makes the read one-shot per instance. Pre-fix: WARNING: DATA RACE, write at provider.go:187 against read at provider.go:280. TestAzureProvider_GetRegions and TestAzureProvider_GetServiceClient_WithSubscriptionLookup now clear AZURE_SUBSCRIPTION_ID via the existing helper: their fixtures resolve through the newly-guarded path, so an exported value on a developer machine would fail them for an unrelated reason.
4bc85e5 to
c8f13b3
Compare
|
Rebased onto
No content change from the rebase, just a new base. Verified after rebasing: One note for the record: a stale worktree for this branch still held a pre-rebase copy of the history, which looked like 9 unpushed commits. It was not — the remote already contained all of that work plus two newer commits ( |
Adversarial review record (merged)Independent reviewer, distinct from the author. Two reports. Freshness handled properly rather than trusted. This PR's green checks were six days old, predating four merges that landed the same day, so they were not evidence about current Only two files overlapped The headline feature is genuinely constructed in production, not test-only: Fan-out concurrency: the doc's claim that unbounded goroutines are fine because the ARM semaphore is acquired inside each per-subscription client is true — verified that it is a shared semaphore carried on the context and set by the scheduler, not per-client. Carve-out the comment states but does not weigh: when no semaphore is attached (CLI, tests) The two CodeRabbit findings, both real and both fixed before mergeCR-1, money-adjacent: CR-2: Consequence of this merge, trackedThe partial-sweep eviction hazard (#1652) becomes structurally closer with the fan-out client shipped: eviction is keyed on Note the existing tests encode that hazard as intended behaviour — |
…loses #553) (#1520) * feat(azure): org-wide multi-subscription recommendation collection Cache the ARM subscriptions list on AzureProvider (getOrFetchAccounts, double-checked locking, InvalidateAccountsCache for tests) so repeated GetAccounts/GetServiceClient/GetRecommendationsClient calls in one run do not each re-issue the subscriptions.List API call. Add MultiSubscriptionRecommendationsClient, which fans recommendation collection out across every subscription visible to the authenticated principal via errgroup, isolating per-subscription errors and only failing when every subscription fails. GetRecommendationsClient now returns this fan-out client when no subscription is pinned and 2+ are discovered, bringing Azure to parity with the AWS provider's automatic whole-organization recommendation coverage. Pinned-subscription and single-subscription paths are unchanged. Closes #553 * fix(azure): singleflight the accounts cache instead of locking across the ARM fetch getOrFetchAccounts held accountsMu.Lock() for the entire subscriptions.List network round-trip via fetchAccountsLocked, so every reader of the cached subscription list blocked behind a slow or hung ARM call. Move the fetch into golang.org/x/sync/singleflight so concurrent cold-cache callers coalesce into one in-flight request, taking accountsMu only to read or populate cachedAccounts. Add a deterministic regression test that stages N goroutines through a channel-gated fake fetch and asserts exactly one underlying ARM call. * fix(azure): keep recommendation scope pinned and fix cache invalidation Adversarial review of the org-wide fan-out. Scoping first. GetRecommendationsClient ignored the resolved default subscription. Before fan-out existed, an unpinned provider resolved the default via resolveSubscriptionIDFromCtx, which honours AZURE_SUBSCRIPTION_ID and errors when the scope is ambiguous. The fan-out path replaced that with "2+ visible subscriptions means fan out", so a caller that pinned a subscription through the environment silently started receiving every other visible subscription's recommendations. Restore the precedence: a resolvable default still yields a single-subscription client, a configured AZURE_SUBSCRIPTION_ID naming a subscription this principal cannot see is an error, and only a genuinely unnamed scope fans out. MultiSubscriptionRecommendationsClient ignored params.AccountFilter. The AWS provider applies that filter to every recommendation it returns (filterByAccounts in providers/aws/service_client.go). It was moot while an Azure client could only ever cover its own subscription, but a client covering every visible subscription has to honour it or a request scoped to one account comes back carrying others'. Filter before the fan-out so subscriptions outside the filter are never queried, and error rather than return an empty slice when the filter matches nothing. Then the subscription-list cache. InvalidateAccountsCache was a no-op against an in-flight fetch: a fetch that started before the invalidation still published its pre-invalidation snapshot afterwards, so the next read was served from cache and handed back exactly the data the caller asked to discard, with no second ARM call. A caller arriving in that window also joined the still in-flight pre-invalidation call through the constant singleflight key. Both are closed by an accountsGen counter that keys the singleflight call and gates the cache write. singleflight hands the leader's error to every waiter, so a caller whose own context was still live inherited a cancellation raised by whichever unrelated caller won the leader election. Retry once as leader when our own context is still healthy; our own cancellation stays terminal, so this cannot spin on a dead context. The unchecked type assertion on singleflight's untyped result now returns an error instead of panicking. Test fixes: the context-cancellation test passed with the guard it named deleted, because fakes that return ctx.Err() make every subscription fail and the all-failed guard produces the same error either way; its fakes now ignore the cancelled context so only the post-Wait ctx.Err() check can produce the error. The new cache tests read AZURE_SUBSCRIPTION_ID from the ambient environment and now neutralise it, the concurrent cold-cache test waits for every follower to enter GetAccounts rather than relying on a runtime.Gosched nudge, and the constructor test asserts the built client is actually stored. Each behaviour has a regression test verified to fail with its fix reverted. Refs #553 * fix(scheduler): reject Azure accounts with no subscription ID collectAzureForAccount tags every recommendation it returns with the registered account's UUID, so the provider has to be pinned to that account's subscription. It built the provider from acct.AzureSubscriptionID without checking it was set, and validateCloudAccountRequest does not require the field, so an azure row created without one produced an unpinned provider. That was harmless while an unpinned provider resolved a single subscription or errored. With the org-wide fan-out it instead collects from every subscription the credential can see, and those results are then filed under this one account, where anyone authorised for it can read them. Reject the misconfigured row by name instead. * fix(azure): drop the accounts cache when the credential is swapped SetCredential is used in production, not only in tests: the scheduler and purchase-execution paths construct the provider and then install per-account federated credentials. The subscription-list cache added in this PR records what the previous credential could see, so leaving it in place across a swap would report subscriptions the new principal may have no access to, and GetRecommendationsClient would fan out across them. Every caller today installs the credential before the first accounts fetch, so this is a guard against a future reordering rather than a live leak. SetSubscriptionsClient gets the same treatment for the same reason. * fix(azure): validate AZURE_SUBSCRIPTION_ID before default resolution CodeRabbit finding. GetRecommendationsClient consulted getDefaultSubscriptionID before validating the configured target, and resolveDefaultSubscription's rule 3 marks a lone visible subscription as the default even when a target was configured and did not match it. So with AZURE_SUBSCRIPTION_ID naming a subscription the credential cannot see and exactly one subscription visible, that one subscription was returned as the default and the fail-loud check further down was unreachable. An operator who typo'd the variable, or whose credential lost access to the intended subscription, silently collected against the wrong subscription instead of getting an error: a misconfiguration answered with a plausible wrong result. Validate the target against the discovered accounts first, via a direct membership check rather than IsDefault, so it fails loud regardless of how many subscriptions are visible. A target that IS visible is honoured directly. The previous test only covered two visible subscriptions, where rule 3 never fires; the new subtest pins the single-visible-subscription case and fails against the old ordering. * fix(azure): report partial multi-subscription sweeps as partial mergeSubscriptionResults returned a nil error whenever at least one subscription succeeded, so 3 of 50 subscriptions failing handed back 47 subscriptions' recommendations indistinguishably from a complete sweep. The only signal was a log line, which no programmatic caller reads. On a path whose output is persisted and rendered as a savings opportunity, an under-collected sweep then reads as a shrinking opportunity rather than a failed one. Add PartialSubscriptionFailureError, returned ALONGSIDE the successful subscriptions' recommendations, carrying the attempted/succeeded counts and every failed subscription with its cause. Unwrap exposes the per-subscription causes so errors.Is still matches through it. Failing the whole sweep on one transient error would be worse than a partial result, so the data is still returned; callers opt in to keeping it by inspecting the error with errors.As, and a caller that treats any non-nil error as fatal now fails loud instead of silently under-reporting. Both outcomes beat the previous silent success. Updated the two call sites that can reach the fan-out (the CLI paths, which are the only ones that construct an unpinned Azure provider) to keep the partial data and report the gap. The scheduler's Azure paths always pin a subscription -- collectAzureAmbient via AZURE_SUBSCRIPTION_ID and collectAzureForAccount via the account's subscription -- so they never build the fan-out client and need no change. Tests assert the distinguishing property directly: a sweep where a subscription was never queried and a complete sweep that found nothing both produce an empty slice, and only the error tells them apart. Verified failing against the old nil-error contract. * fix(scheduler): keep partial multi-subscription sweep data Second-order effect of returning a typed partial-failure error. Every recommendation-fetch site in the scheduler has the shape recs, err := recClient.GetAllRecommendations(ctx) if err != nil { return nil, ... } which discards recs. Correct for a real error, but for a partial sweep it turned one flaky subscription out of fifty into a total collection outage: strictly worse than the silent under-collection the typed error exists to prevent. The diff that introduced it looked like a pure improvement in isolation and only misbehaved at callers that were not re-read. Add tolerateIncompleteSweep and route every fetch site through it, so the policy lives in one place instead of relying on each call site to remember it. A partial sweep logs a warning naming the subscriptions that were not queried and returns nil, so the data that was collected is kept and persisted. Any other error is returned unchanged. Also export AsPartialSubscriptionFailure from the azure provider so the scheduler and the CLI share one definition of the check rather than each hand-rolling errors.As, and add FailedSubscriptionIDs for the log lines. Recording the incompleteness in the state table's last_collection_error, so the dashboard shows the sweep as partial rather than merely smaller, needs a partial-note threaded through collectProviderRecommendations and its three per-provider implementations. Left as follow-up rather than bolted on here; the warning names the failed subscriptions in the meantime. Tests drive fetchAndConvert with a client returning recommendations plus a partial-failure error and assert the data survives, that a genuine error still fails loud, and that the helper is narrow. Verified failing with the tolerance removed. * chore(scheduler): use US spelling of "behavior" in two comments The Lint Code job pins golangci-lint v2.10.1 and .golangci.yml sets misspell locale to US, so the British spelling in these two comments failed the lint gate and cascaded to the CI Success aggregate: internal/scheduler/scheduler.go:910 internal/scheduler/scheduler_test.go:766 Both are comment prose, so this changes no identifier, no database column name and no string any test asserts on. Scoped deliberately to the two lines the linter flagged. 49 further occurrences of the British spelling remain elsewhere in the tree and were left alone: they are pre-existing, unrelated to this branch, and a repo-wide rewrite here would bury a two-word fix under a diff touching dozens of files. * fix(azure): report filter misses and lock credential swaps in fan-out Three review findings on the org-wide multi-subscription fan-out. 1. A partially-satisfied AccountFilter silently narrowed the sweep. A filter matching NO accessible subscription already errored, but one matching some and missing others returned the matched subscriptions' recommendations with a nil error. An operator scoping a sweep to sub-A and sub-B whose principal has lost Reader on sub-B therefore read sub-B as "no savings available" rather than "never queried" -- the exact confusion PartialSubscriptionFailureError exists to prevent, and which selectSubscriptions' own doc comment says must not happen. selectSubscriptions now iterates the filter rather than the discovered subscriptions, so every requested entry is accounted for as matched or missed, and the misses are folded into the partial-failure error as ErrSubscriptionNotAccessible. They count towards Attempted and Failed; the subscriptions that did answer still return their data, so one revoked subscription does not become a total collection outage. The filter is also deduplicated, so a repeated entry is neither queried twice nor double-counted. 2. PartialSubscriptionFailureError.Error() indexed Failed[0] unguarded. The type and its fields are exported, so a zero-value or externally-constructed instance panicked when formatted -- an error that crashes when logged fails exactly when the message is needed. 3. SetCredential and SetSubscriptionsClient wrote cred/subscriptionsClient as plain assignments and only then took accountsMu to invalidate, while fetchAccounts read both from a goroutine running on behalf of every caller that joined the single-flight. Both fields are now published inside one accountsMu critical section (via invalidateAccountsCacheLocked, since RWMutex is not reentrant) and snapshotted together under RLock, which also closes the window where a fetch pairs the old client with the new credential. The remaining in-package reads go through the same accessors so the guarantee holds rather than covering only one path. No production caller is concurrent today, so this was latent. Both behavioral fixes are covered by tests confirmed to fail on the pre-fix code: the partial-filter test with "An error is expected but got nil", and the swap test with a data race reported by -race. Refs #1652, #1653, #1654 * fix(azure): validate configured subscription on the regions/service path Addresses both CodeRabbit findings from the review round on PR #1520. Both harden invariants that the previous commit established on one path and left off the others, which reads as a global guarantee but is not. 1. Silent subscription retarget (providers/azure/provider.go) resolveSubscriptionIDFromCtx -- which backs GetRegions and GetServiceClient -- called getDefaultSubscriptionID without first validating an explicitly configured subscription against the discovered list. resolveDefaultSubscription deliberately falls through to its single-visible-subscription rule when the configured target is not found, so with an unreachable AZURE_SUBSCRIPTION_ID (or ProviderConfig subscription) and exactly one visible subscription, the request was silently retargeted to whichever subscription happened to be visible. GetRecommendationsClient already guarded this via accountsContain under a comment reading "This ordering is load-bearing"; the same misconfiguration therefore errored on the recommendations path and silently retargeted on the regions/service path. GetServiceClient drives the resource enumeration recommendations are computed from, and is also the purchase-execution path's service-client lookup, so the operator could believe they were scoped to subscription A while data came from B. The guard is extracted into validateConfiguredSubscription and reused by GetRecommendationsClient, so the two paths cannot drift apart again. It covers both configuration axes (ProviderConfig field and the env var), mirroring resolveDefaultSubscription's own priority order. Production Azure flows pin AzureSubscriptionID (scheduler, registration and account handlers) and short-circuit before this path, so the guard engages only on the unpinned/ambient path where the retarget lived. GetRegions' error wrapper no longer claims "no Azure subscriptions found": the new inner error fires precisely when subscriptions were found but the configured one is not among them. 2. Unsynchronized credProvider (providers/azure/provider.go) SetCredentialProvider wrote p.credProvider outside accountsMu while IsConfigured read it inside credOnce.Do -- the one swappable field left outside the locking discipline the rest of the provider follows. Latent today (SetCredentialProvider has no production callers), but a half-kept invariant reads as a kept one. Published and read under accountsMu now, matching cred and subscriptionsClient. It deliberately does not invalidate the accounts cache: credProvider only feeds the lazy resolution that runs when no credential was installed at all, so no subscription list resolved under a different credential can exist yet. Regression tests, both confirmed failing against the pre-fix code: - TestAzureProvider_ResolveSubscription_InvisibleConfiguredTargetErrors uses an invisible configured target with exactly ONE visible subscription -- the case the fall-through hides. With two or more visible subscriptions getDefaultSubscriptionID returns "" and the pre-existing "multiple Azure subscriptions" error fires either way, so such a test would pass with or without the guard. Covers GetRegions and GetServiceClient, both configuration axes, and the honoured visible-target case. Pre-fix: 3 of 4 subtests fail with "An error is expected but got nil". - TestAzureProvider_ConcurrentCredentialProviderSwapAndIsConfigured_NoDataRace loops over fresh providers because credOnce makes the read one-shot per instance. Pre-fix: WARNING: DATA RACE, write at provider.go:187 against read at provider.go:280. TestAzureProvider_GetRegions and TestAzureProvider_GetServiceClient_WithSubscriptionLookup now clear AZURE_SUBSCRIPTION_ID via the existing helper: their fixtures resolve through the newly-guarded path, so an exported value on a developer machine would fail them for an unrelated reason.
1 participant
Summary
AzureProvider(getOrFetchAccounts, single-flight underaccountsMu sync.RWMutex) soGetAccounts,GetServiceClient, andGetRecommendationsClientdon't each re-issue thesubscriptions.ListAPI call within one logical runMultiSubscriptionRecommendationsClient, which fans recommendation collection out across every subscription visible to the authenticated principal (errgroup, per-subscription error isolation, all-attempted-failed guard, typed partial-failure error)GetRecommendationsClientreturns the fan-out client when no subscription is pinned and 2+ subscriptions are discovered. Pinned-subscription and single-discovered-subscription paths are unchanged.Nothing in this 1787-line diff changes what CUDly does when deployed as it is currently wired. The fan-out is scaffolding for a follow-up that connects it; it is not a shipped feature. All three integration surfaces it adds are unreachable:
MultiSubscriptionRecommendationsClientitself. It is only returned byGetRecommendationsClienton the unpinned path, and every AzureGetRecommendationsClientcall site reaches it through a pinned provider:internal/scheduler/scheduler.go:685(collectAzureAmbient) — the subscription comes fromAZURE_SUBSCRIPTION_ID, and the caller at:752returns early when that is empty, so the provider is always pinned.internal/scheduler/scheduler.go:799(collectAzureForAccount) — guarded by the explicitacct.AzureSubscriptionID == ""fail-fast at:782.internal/purchase/execution.go:476builds an Azure provider fromaccount.AzureSubscriptionIDwith no empty-guard, but that provider is used for purchase execution only — it never reachesGetRecommendationsClient.pkg/provider/registry.go:82-93stores provider factories, not instances, so there is no shared unpinned provider a caller could pick up either.The Azure partial-failure handling in
cmd/multi_service_helpers.go(:103-111and:449-459). Dead:recClientin that file is always the AWS adapter (cmd/multi_service.go:122), anddiscoverRegionsForServiceis reachable only via an AWSDescribeRegionsfailure. These branches add acmd→providers/azureimport for code no input can reach, and carry nocmd-level tests.tolerateIncompleteSweep(internal/scheduler/scheduler.go:906-925). A no-op at all five of its call sites: every Azure path pins a subscription, and GCP/AWS never return*azure.PartialSubscriptionFailureError.What would wire it up: a caller that constructs
AzureProviderwithoutAzureSubscriptionID/Profile— e.g. an org-wide collection run for a service principal holding Reader across many subscriptions, rather than today's per-registered-account loop. That change is deliberately out of scope here: it is a collection-model decision with its own persistence consequences (tracked in #1652), not a mechanical follow-on.Reading the test suite alone would reasonably suggest the feature is live. It is not. This section exists so a future maintainer does not have to rediscover that.
Why fan-out instead of a single org-wide API call
AWS Cost Explorer's
AccountScope=Linkedcovers an entire organization in one API call. Azure has no equivalent — the Consumption Reservation Recommendations and Advisor APIs are subscription-scoped — so achieving org-wide coverage requires calling the per-subscriptionRecommendationsClientAdapteronce per subscription and aggregating client-side. This mirrors the design proposed in #561 (closed as stale; reimplemented fresh against currentmainhere since that branch's diff no longer applies cleanly).What changed (per file)
providers/azure/provider.goaccountsMu sync.RWMutex/cachedAccounts/accountsGen/accountsSFfieldsgetOrFetchAccounts(single-flight cache, returns a defensive copy viacloneAccountsso a caller mutating the returned slice can't corrupt the cache) andfetchAccounts(the actual ARM call, extracted from the previousGetAccountsbody)GetAccountsdelegates togetOrFetchAccounts— identical external behavior, now cache-backedInvalidateAccountsCache()for tests/long-lived callers that need a forced refreshGetRecommendationsClient: pinned subscription unchanged; unpinned path resolves accounts via the cache, validates an explicitly configuredAZURE_SUBSCRIPTION_IDagainst the discovered list before any default resolution, and returnsMultiSubscriptionRecommendationsClientonly when the scope is genuinely ambiguousSetCredential/SetSubscriptionsClientpublish the swapped field underaccountsMuand invalidate in the same critical section;fetchAccountssnapshots both fields underRLockproviders/azure/recommendations_multi_subscription.go(new)MultiSubscriptionRecommendationsClientimplementingprovider.RecommendationsClientNewMultiSubscriptionRecommendationsClientrejects an empty account list and fails loud if building any per-subscription client errorsGetRecommendationsfans out via errgroup; each subscription's error is isolated (logged, doesn't cancel siblings); explicit post-Waitctx.Err()check propagates parent-context cancellation; if every subscription fails, returns a wrapped error instead of a silently empty resultPartialSubscriptionFailureErrorreturns the successful subscriptions' data alongside a typed error naming what was missed, so an incomplete sweep is distinguishable from a complete one that found nothingselectSubscriptionshonoursAccountFilter, and reports filter entries that match no accessible subscription (ErrSubscriptionNotAccessible) rather than silently narrowing the sweepGetRecommendations; adding a bound at this layer for callers that supply no semaphore is tracked in fix(azure): bound concurrency in the multi-subscription recommendations fan-out cloud-commitments-go#58Tests —
providers/azure/provider_test.go,providers/azure/recommendations_multi_subscription_test.goAZURE_SUBSCRIPTION_IDerrors)Known follow-ups (filed, not fixed here)
accountsis not deduped by subscription ID, so a duplicate would double-count that subscription's savings (unverified whether ARM can emit duplicates)Test plan
go build ./...go vet ./providers/azure/... ./internal/scheduler/... ./cmd/go test -race -count=1 ./providers/azure/...go test -race -count=1 ./internal/scheduler/... ./cmd/...golangci-lint run --timeout=10mat the CI-pinnedv2.10.1— 0 issuesgocyclo -over 10onproviders/azure internal/scheduler cmd— cleangofmt -l/goimports -lclean on touched filesGetRecommendationsClientwithout pinning a subscription — verify recommendations from both subscriptions are returned. Not run: no such path exists in the deployed product yet (see the no-op section above), so this is exercised by unit tests only.Closes #553
Refs #473, #561, #1652, LeanerCloud/cloud-commitments-go#58, LeanerCloud/cloud-commitments-go#59
Summary by CodeRabbit