fix(azure): make location optional for spot allocation#840
Conversation
| } | ||
| return nil, fmt.Errorf("missing default value for Azure Location, set ARM_LOCATION_NAME or AZURE_DEFAULTS_LOCATION environment variable") | ||
| logging.Infof("missing default value for Azure Location, if needed it should set using ARM_LOCATION_NAME or AZURE_DEFAULTS_LOCATION") | ||
| return nil, nil |
There was a problem hiding this comment.
I would expect a check in the options to see if the execution is for spot or not,
- No spot the location is mandatory as it should be used as the target location for the on demand. So error
- Spot it will return nil
There was a problem hiding this comment.
Added ProviderOptionalLocation() — sets locationOptional: true on the Azure struct. DefaultHostingPlace() checks that flag before reading any env vars, so spot runs never get pinned to a location from the environment. Also applied this to all Destroy functions since teardown doesn't need a location and failing there would leave resources running.
rishupk
force-pushed
the
fix+azure-spot-location-optional
branch
from
353407c to
f640601
Compare
|
Warning Review limit reached
More reviews will be available in 11 minutes and 31 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a ChangesAzure Spot Provider Selection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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)
pkg/provider/azure/azure.go (1)
42-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSpot mode should short-circuit before reading location env vars.
DefaultHostingPlace()still returns an env-derived location for spot runs whenARM_LOCATION_NAME/AZURE_DEFAULTS_LOCATIONis set. Sincemc.InitwritestargetHostingPlacewhenever this return is non-nil, spot flows can get unintentionally pinned instead of letting SpotInfo choose location.Suggested fix
func (a *Azure) DefaultHostingPlace() (*string, error) { + if a.spot { + logging.Info("Spot allocation enabled: skipping default Azure location resolution") + return nil, nil + } + hp := os.Getenv("ARM_LOCATION_NAME") if len(hp) > 0 { return &hp, nil } hp = os.Getenv("AZURE_DEFAULTS_LOCATION") if len(hp) > 0 { return &hp, nil } - if a.spot { - return nil, nil - } return nil, fmt.Errorf("missing default value for Azure Location, set ARM_LOCATION_NAME or AZURE_DEFAULTS_LOCATION environment variable") }🤖 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 `@pkg/provider/azure/azure.go` around lines 42 - 53, The spot mode check in the DefaultHostingPlace method should occur before attempting to read the environment variables ARM_LOCATION_NAME and AZURE_DEFAULTS_LOCATION. Move the spot mode check (if a.spot { return nil, nil }) to the beginning of the function, before reading any environment variables, so that spot runs short-circuit immediately without being pinned to a location derived from these env vars.
🤖 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 `@pkg/provider/azure/azure.go`:
- Around line 42-53: The spot mode check in the DefaultHostingPlace method
should occur before attempting to read the environment variables
ARM_LOCATION_NAME and AZURE_DEFAULTS_LOCATION. Move the spot mode check (if
a.spot { return nil, nil }) to the beginning of the function, before reading any
environment variables, so that spot runs short-circuit immediately without being
pinned to a location derived from these env vars.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: b9a359d2-c21e-4960-8f98-8ad57c839b92
📒 Files selected for processing (5)
pkg/provider/azure/action/aks/aks.gopkg/provider/azure/action/kind/kind.gopkg/provider/azure/action/linux/linux.gopkg/provider/azure/action/windows/windows.gopkg/provider/azure/azure.go
rishupk
force-pushed
the
fix+azure-spot-location-optional
branch
from
f640601 to
44fbb3d
Compare
…ructor - Add locationOptional bool to Azure struct and ProviderOptionalLocation() constructor; DefaultHostingPlace() short-circuits before reading env vars when locationOptional is true, so spot runs are never pinned to a location - Spot Create: uses ProviderOptionalLocation() — SpotInfo picks the location - Destroy: uses ProviderOptionalLocation() — no location needed for teardown - On-demand Create: uses Provider() — errors if no location env is set Fixes redhat-developer#839. Co-Authored-By: Claude <Sonnet 4.6> <noreply@anthropic.com> Signed-off-by: Rishabh Kothari <rkothari@redhat.com>
rishupk
force-pushed
the
fix+azure-spot-location-optional
branch
from
949eab4 to
bc96857
Compare
2 participants
Azure spot allocation finds its own location, so there's no reason to require one at init time. A fix in #826 (commit 7c5d50d) changed
DefaultHostingPlace()to hard-error when no location env var is set — that worked fine for on-demand but also fired for spot, breaking workflows that never needed it (see #839).This restores the original
nil, nilreturn with an info log. The non-spot path inallocation.goalready enforces location when it actually matters.Fixes #839.
Co-Authored-By: Claude <Sonnet 4.6> noreply@anthropic.com
Signed-off-by: Rishabh Kothari rkothari@redhat.com