Fix VM scaling: use ScaleVirtualMachine instead of ChangeServiceForVirtualMachine - #323
Open
sureshanaparti wants to merge 9 commits into
Open
Fix VM scaling: use ScaleVirtualMachine instead of ChangeServiceForVirtualMachine#323sureshanaparti wants to merge 9 commits into
sureshanaparti wants to merge 9 commits into
Conversation
…rtualMachine When the service_offering of an instance changes, the provider must call ScaleVirtualMachine to actually scale the VM's CPU and memory resources. The previous implementation used ChangeServiceForVirtualMachine which only updated CloudStack metadata without scaling the actual VM. This change ensures that when a service offering is updated, the VM is properly scaled with the new compute resources. Fixes: #273
sureshanaparti
requested review from
sudo87 and
vishesh92
and
a lite review from Copilot
August 17, 2026 11:07
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the cloudstack_instance update path so that changing service_offering triggers an actual VM scale operation in CloudStack (CPU/memory), aligning behavior with CloudStack’s scaling semantics and addressing the core symptom described in issue #273.
Changes:
- Replace
ChangeServiceForVirtualMachinewithScaleVirtualMachinewhenservice_offeringchanges. - Update related log messaging and error text for the new scaling operation.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+700
to
+701
| return fmt.Errorf( | ||
| "Error changing the service offering for instance %s: %s", name, err) | ||
| "Error scaling the service offering for instance %s: %s", name, err) |
Comment on lines
+694
to
+698
| // Create a new parameter struct for scaling | ||
| p := cs.VirtualMachine.NewScaleVirtualMachineParams(d.Id(), serviceofferingid) | ||
|
|
||
| // Change the service offering | ||
| _, err = cs.VirtualMachine.ChangeServiceForVirtualMachine(p) | ||
| // Scale the VM to the new service offering | ||
| _, err = cs.VirtualMachine.ScaleVirtualMachine(p) |
Comment on lines
+697
to
+698
| // Scale the VM to the new service offering | ||
| _, err = cs.VirtualMachine.ScaleVirtualMachine(p) |
Contributor
|
The ScaleVirtualMachine swap for service_offering looks correct, but I don't think this closes #273. The issue is specifically about the details map (cpuNumber/cpuSpeed/memory) — that update path (resourceCloudStackInstanceUpdate, d.HasChange("details") block) is untouched and still calls plain UpdateVirtualMachine, which only updates metadata without resizing the VM. Can this be extended to call ScaleVirtualMachine there too? Also, no tests were added for either path. |
Review changes: - Enhanced error messages to include the target service offering for better debugging - Added support for scaling when compute-related details change (cpuNumber, cpuSpeed, memory) without requiring a service offering change - Enhanced test assertions to validate actual CPU/memory values after scaling, ensuring ScaleVirtualMachine was invoked (not just metadata updates)
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
cloudstack/resource_cloudstack_instance.go:707
- The compute-details scaling logic is nested inside the outer "Attributes that require reboot" block, but that block does not include
d.HasChange("details")in its condition. As a result, changing onlydetailswill skip this ScaleVirtualMachine path entirely (and only run UpdateVirtualMachine(details) later), which doesn’t satisfy the dynamic-scaling behavior described in issue #273.
// Check if compute-related details have changed and scale the VM
if d.HasChange("details") {
cloudstack/resource_cloudstack_instance.go:728
- This conversion assumes every value in
detailsis a string (v.(string)). The instance schema definesdetailsasTypeMapwithout anElem, so Terraform configs can supply non-string values (numbers/bools), which would panic here at runtime. Consider converting viafmt.Sprint(v)(or enforcingElem: &schema.Schema{Type: schema.TypeString}in the schema).
detailsForAPI := make(map[string]string)
for k, v := range newDetailsMap {
detailsForAPI[k] = v.(string)
}
cloudstack/resource_cloudstack_instance_test.go:380
- This test doesn’t actually validate that scaling applied the expected CPU/memory for the "Medium Instance" offering; it only checks that the fields are non-empty, and it compares them to empty strings (which may be the wrong type if these fields are numeric). Consider asserting concrete expected CPU/memory values (or reading the service offering and comparing VM CPU/memory against it).
// Verify that ScaleVirtualMachine was actually invoked by checking that
// the VM's CPU and memory match the Medium Instance service offering.
// This ensures the scaling operation completed successfully, not just
// the metadata update.
if instance.Cpunumber == "" {
return fmt.Errorf("CPU number is empty - VM scaling may not have completed")
}
if instance.Memory == "" {
return fmt.Errorf("Memory is empty - VM scaling may not have completed")
}
Comment on lines
+708
to
+710
| oldDetails, newDetails := d.GetChange("details") | ||
| oldDetailsMap := oldDetails.(map[string]interface{}) | ||
| newDetailsMap := newDetails.(map[string]interface{}) |
Comment on lines
+730
to
+731
| p := cs.VirtualMachine.NewScaleVirtualMachineParams(d.Id(), "") | ||
| p.SetDetails(detailsForAPI) |
The Cpunumber and Memory fields are numeric (int64), not strings. Comparing them to empty strings causes a Go type mismatch error during build. Changed assertions to compare with 0 instead, checking that these values are properly set after the scaling operation.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cloudstack/resource_cloudstack_instance.go:717
- This
details-based scaling logic is nested inside the "Attributes that require reboot" block. If a user changes onlydetails(e.g.cpuNumber/cpuSpeed/memory) without changing any of the reboot-gated fields, this scaling code will never run; later in the functionUpdateVirtualMachinestill updatesdetailsmetadata (seeresource_cloudstack_instance.go:906-917), so the original behavior from #273 remains for that case.
// Check if compute-related details have changed and scale the VM
if d.HasChange("details") {
cloudstack/resource_cloudstack_instance.go:722
detailsis declared asschema.TypeMapwithoutElem, so its values are not guaranteed to bestring. As written, the unchecked map assertions (oldDetails.(map[string]interface{})) can panic when the old/new value isnil, andv.(string)can panic when users provide non-string values (e.g. numbers). Also, passing an empty service offering ID toNewScaleVirtualMachineParamsis risky; reusing the currentservice_offeringID avoids sending an empty value.
oldDetails, newDetails := d.GetChange("details")
oldDetailsMap := oldDetails.(map[string]interface{})
newDetailsMap := newDetails.(map[string]interface{})
// Check if any compute-related details changed (cpuNumber, cpuSpeed, memory)
cloudstack/resource_cloudstack_instance.go:712
fmt.Errorf(errMsg)treatserrMsgas a format string. If the CloudStack error text contains%(or other formatting verbs), this can produce malformed output or unexpected formatting. Use a constant format string instead (orerrors.New).
return fmt.Errorf(errMsg)
cloudstack/resource_cloudstack_instance_test.go:379
- The added assertions only check that
Cpunumber/Memoryare non-zero, which doesn’t demonstrate that scaling actually happened (those fields are typically set for any VM). A stronger check is to fetch the expected CPU/memory from the target service offering and compare them to the VM’s reported values.
// Verify that ScaleVirtualMachine was actually invoked by checking that
// the VM's CPU and memory are set. This ensures the scaling operation
// completed successfully, not just the metadata update.
if instance.Cpunumber <= 0 {
return fmt.Errorf("CPU number not set - VM scaling may not have completed, got: %d", instance.Cpunumber)
sureshanaparti
force-pushed
the
feature/issue-273-vm-scaling
branch
2 times, most recently
from
August 17, 2026 13:14
15858a2 to
b4230bc
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
cloudstack/resource_cloudstack_instance.go:710
d.GetChange("details")can returnnil(or a non-map) for TypeMap attributes; the direct type assertions tomap[string]interface{}will panic in that case. The existing code path that updates details later in this function already guardsd.Get("details") != nil, so this block should handle nil/empty maps similarly.
oldDetails, newDetails := d.GetChange("details")
oldDetailsMap := oldDetails.(map[string]interface{})
newDetailsMap := newDetails.(map[string]interface{})
cloudstack/resource_cloudstack_instance.go:707
- This compute-details scaling block is nested inside the "Attributes that require reboot to update" section (the outer
ifthat does not included.HasChange("details")). As a result, updates that only changedetails(e.g.,details.cpuNumber,details.memory) will skipScaleVirtualMachineentirely and fall back toUpdateVirtualMachinelater (which was the original bug).
// Check if compute-related details have changed and scale the VM
if d.HasChange("details") {
cloudstack/resource_cloudstack_instance.go:732
NewScaleVirtualMachineParams(d.Id(), "")drops the service offering ID entirely. Even when scaling viadetails, the VM still has a service offering, and CloudStack typically requiresserviceofferingidforscaleVirtualMachine. Passing an empty string risks API errors or no-op scaling.
p := cs.VirtualMachine.NewScaleVirtualMachineParams(d.Id(), "")
p.SetDetails(detailsForAPI)
cloudstack/resource_cloudstack_instance_test.go:375
- The new assertions (
Cpunumber > 0/Memory > 0) don’t actually verify that scaling happened as part of the resize, since those fields are typically populated for any VM (including before the update). The test should assert that CPU/memory changed relative to the pre-resize values or match the expected values for the "Medium Instance" service offering.
if instance.Cpunumber <= 0 {
return fmt.Errorf("CPU number not set - VM scaling may not have completed, got: %d", instance.Cpunumber)
}
Comment on lines
+726
to
+728
| for k, v := range newDetailsMap { | ||
| detailsForAPI[k] = v.(string) | ||
| } |
- Add nil-safety checks for d.GetChange("details") to prevent panics
- Use safe value stringification (fmt.Sprintf) for detail map values
- Get current service offering ID for details-only scaling
- Improve error message to show service offering transition
- Add comprehensive test case for VM scaling with compute detail validation
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cloudstack/resource_cloudstack_instance.go:703
- This error message also uses %s for old/new offerings (interface{}), which will format incorrectly. Use %v (or cast) so the message is readable.
return fmt.Errorf(
"Error scaling VM %s from %s to %s: %s", name, oldOffering, newOffering, err)
}
cloudstack/resource_cloudstack_instance.go:687
- The log message formats old/new service offering values with %s, but d.GetChange returns interface{} values; this will produce
%!s(...)in logs. Use %v or cast to string before formatting.
This issue also appears on line 701 of the same file.
oldOffering, newOffering := d.GetChange("service_offering")
log.Printf("[DEBUG] Service offering changed for %s from %s to %s, starting scale", name, oldOffering, newOffering)
cloudstack/resource_cloudstack_instance_test.go:438
- The current assertions in this helper (
Cpunumber > 0/Memory > 0) don’t distinguish successful scaling from the previous metadata-only update; VMs normally have CPU/memory > 0 even without scaling. Make the test assert CPU/memory match the target service offering values.
// Verify that the VM has actually been scaled by checking CPU and memory
// are set to the Medium Instance values. This ensures ScaleVirtualMachine
// was invoked and completed successfully.
if instance.Cpunumber <= 0 {
return fmt.Errorf("CPU number not set after scaling, got: %d", instance.Cpunumber)
}
if instance.Memory <= 0 {
return fmt.Errorf("Memory not set after scaling, got: %d", instance.Memory)
}
// Medium Instance should have more resources than Small Instance
// (this is environment-dependent, but at minimum both should be > 0)
if instance.Serviceofferingname != "Medium Instance" {
return fmt.Errorf("Bad service offering after scaling: %s", instance.Serviceofferingname)
cloudstack/resource_cloudstack_instance.go:708
- The compute-details scaling block is nested under the "Attributes that require reboot to update" branch, but that outer condition does not include
details. If onlydetailschanges, this scaling code will never run, and the update will fall back to UpdateVirtualMachine (metadata) only.
// Check if compute-related details have changed and scale the VM
if d.HasChange("details") {
oldDetails, newDetails := d.GetChange("details")
sureshanaparti
commented
Aug 18, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cloudstack/resource_cloudstack_instance.go:756
- This compute-details scaling logic is inside the stop/start block guarded by changes to name/service_offering/affinity groups/keypairs/userdata. Since that guard does not include "details", changing only details.cpuNumber/cpuSpeed/memory will still skip ScaleVirtualMachine and only run the UpdateVirtualMachine(details) path later (metadata-only), which does not satisfy the dynamic scaling behavior described in issue #273.
// Check if compute-related details have changed and scale the VM
if d.HasChange("details") {
oldDetails, newDetails := d.GetChange("details")
cloudstack/resource_cloudstack_instance_test.go:496
- This test helper doesn't verify that the VM's CPU/memory match the new service offering; it only checks they are > 0 and that the offering name changed. That can pass even if only CloudStack metadata changed (the original bug).
// Verify that the VM has actually been scaled by checking CPU and memory
// are set to the Medium Instance values. This ensures ScaleVirtualMachine
// was invoked and completed successfully.
if instance.Cpunumber <= 0 {
return fmt.Errorf("CPU number not set after scaling, got: %d", instance.Cpunumber)
}
if instance.Memory <= 0 {
return fmt.Errorf("Memory not set after scaling, got: %d", instance.Memory)
}
// Medium Instance should have more resources than Small Instance
// (this is environment-dependent, but at minimum both should be > 0)
if instance.Serviceofferingname != "Medium Instance" {
return fmt.Errorf("Bad service offering after scaling: %s", instance.Serviceofferingname)
}
cloudstack/resource_cloudstack_instance.go:796
- For details-based scaling, the service offering ID is looked up via retrieveID("service_offering", ...) which does not apply the zone filter used elsewhere (retrieveServiceOfferingID). In multi-zone setups with duplicate offering names, this can scale the VM to the wrong offering or fail to find the correct one.
// Get current service offering ID for details-only scaling
currentServiceOfferingID, e := retrieveID(cs, "service_offering", d.Get("service_offering").(string))
if e != nil {
return e.Error()
}
p := cs.VirtualMachine.NewScaleVirtualMachineParams(d.Id(), currentServiceOfferingID)
p.SetDetails(detailsForAPI)
cloudstack/resource_cloudstack_instance_test.go:472
- These assertions only check that CPU and memory are > 0, which would typically be true even if only the service offering metadata changed. This does not actually validate that ScaleVirtualMachine applied the new offering's compute resources.
This issue also appears on line 481 of the same file.
// Verify that ScaleVirtualMachine was actually invoked by checking that
// the VM's CPU and memory are set. This ensures the scaling operation
// completed successfully, not just the metadata update.
if instance.Cpunumber <= 0 {
return fmt.Errorf("CPU number not set - VM scaling may not have completed, got: %d", instance.Cpunumber)
}
if instance.Memory <= 0 {
return fmt.Errorf("Memory not set - VM scaling may not have completed, got: %d", instance.Memory)
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cloudstack/resource_cloudstack_instance.go:756
- The compute-details scaling logic is inside the “Attributes that require reboot to update” block, but that block isn’t entered when only
detailschanges. As a result, changingdetails.cpuNumber/cpuSpeed/memorywithout also changingname/service_offering/etc will never callScaleVirtualMachine, which is the behavior the linked issue asks for.
// Check if compute-related details have changed and scale the VM
if d.HasChange("details") {
oldDetails, newDetails := d.GetChange("details")
cloudstack/resource_cloudstack_instance.go:797
- The service offering lookup used for details-only scaling is not zone-aware:
retrieveID(cs, "service_offering", ...)usesGetServiceOfferingIDand can return the wrong offering when names are duplicated across zones. This function already usesretrieveServiceOfferingID(cs, zoneid, ...)elsewhere to avoid that ambiguity; this block should do the same.
// Get current service offering ID for details-only scaling
currentServiceOfferingID, e := retrieveID(cs, "service_offering", d.Get("service_offering").(string))
if e != nil {
return e.Error()
}
p := cs.VirtualMachine.NewScaleVirtualMachineParams(d.Id(), currentServiceOfferingID)
p.SetDetails(detailsForAPI)
cloudstack/resource_cloudstack_instance_test.go:376
- This test step doesn’t record the VM’s pre-scale CPU/memory, so the follow-up step can’t assert that scaling actually changed compute resources (the original bug could still pass). Capture
instance.Cpunumber/instance.Memoryafter the initial apply so the next step can compare.
Config: testAccCloudStackInstance_scale,
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudStackInstanceExists(
"cloudstack_instance.foobar", &instance),
resource.TestCheckResourceAttr(
cloudstack/resource_cloudstack_instance_test.go:386
- As written, this step’s checks can still pass if only the service offering metadata changed. Add an explicit assertion that CPU/memory changed compared to the baseline captured in the first step.
Config: testAccCloudStackInstance_scaleUp,
Check: resource.ComposeTestCheckFunc(
testAccCheckCloudStackInstanceExists(
"cloudstack_instance.foobar", &instance),
testAccCheckCloudStackInstanceScaled(&instance),
Contributor
|
The panic/nil-safety fixes look good, thanks. But the core issue is still open: |
d.HasChange("details") was not part of the outer condition gating the
stop/scale/start block, so a details-only change (cpuNumber, cpuSpeed,
memory) skipped ScaleVirtualMachine entirely and fell through to the
old UpdateVirtualMachine-only path, leaving the VM unscaled. Adding
"details" to that condition and removing the now-redundant metadata-only
update block routes details changes through the same scale path already
used for service_offering changes.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cloudstack/resource_cloudstack_instance.go:956
- The update path no longer applies changed
detailsviaUpdateVirtualMachine. Ifdetailschanges but doesn't triggerScaleVirtualMachine(e.g., non-compute keys), the change will be dropped. Even for compute keys,ScaleVirtualMachinemay not persist arbitrary details metadata. Restore anUpdateVirtualMachinecall fordetailschanges (ideally after scaling, as the issue description suggests).
// Check if the tags have changed and if so, update the tags
if d.HasChange("tags") {
if err := updateTags(cs, d, "UserVm"); err != nil {
return fmt.Errorf("Error updating tags on instance %s: %s", name, err)
}
cloudstack/resource_cloudstack_instance.go:698
- Including
detailsin the "requires reboot" gate stops/starts the VM for anydetailschange, even when the change is metadata-only (or when no compute keys changed). This adds unnecessary downtime and also makesdetailsupdates depend on the reboot path. Consider handlingdetailsupdates/scaling outside of this stop/start block, and only stopping when CloudStack actually requires it (e.g., name/keypair/affinity changes or non-dynamic scaling).
// Attributes that require reboot to update
if d.HasChange("name") || d.HasChange("service_offering") || d.HasChange("details") ||
d.HasChange("affinity_group_ids") || d.HasChange("affinity_group_names") ||
d.HasChange("keypair") || d.HasChange("keypairs") ||
d.HasChange("user_data") || d.HasChange("userdata_id") || d.HasChange("userdata_details") {
cloudstack/resource_cloudstack_instance_test.go:490
- The new scaling assertions don't actually validate that the VM was scaled:
Cpunumber > 0andMemory > 0will be true even before scaling, and would still pass if only metadata changed (the original bug). Strengthen this test to verify the CPU/memory changed (e.g., capture baseline values from the first step and assert they increased, or fetch the target service offering and assert instance CPU/memory matches it).
// Verify that the VM has actually been scaled by checking CPU and memory
// are set to the Medium Instance values. This ensures ScaleVirtualMachine
// was invoked and completed successfully.
if instance.Cpunumber <= 0 {
return fmt.Errorf("CPU number not set after scaling, got: %d", instance.Cpunumber)
}
if instance.Memory <= 0 {
return fmt.Errorf("Memory not set after scaling, got: %d", instance.Memory)
}
cloudstack/resource_cloudstack_instance.go:794
- For details-only scaling, the current code uses
retrieveID(..., "service_offering", ...), which is not zone-filtered. Elsewhere in this resource you intentionally useretrieveServiceOfferingIDto avoid ambiguous offerings across zones. Use the same zone-filtered lookup here to prevent scaling against the wrong service offering when names collide.
// Get current service offering ID for details-only scaling
currentServiceOfferingID, e := retrieveID(cs, "service_offering", d.Get("service_offering").(string))
if e != nil {
return e.Error()
}
The Update path packed every attribute change into inline blocks, several of them deeply nested inside the stop/start reboot branch, making the flow hard to read and each branch impossible to test in isolation. Extract each condition into its own helper: the reboot-required ones (updateInstanceName, updateServiceOffering, updateComputeDetails, updateAffinityGroupIds, updateAffinityGroupNames, updateKeypair, updateUserData, updateUserdataId, updateUserdataDetails) plus updateInstanceTags and updateDeleteProtection. Each helper is a no-op unless its own attribute changed, so the handler now reads as a flat sequence of update steps. No behaviour change.
sureshanaparti
force-pushed
the
feature/issue-273-vm-scaling
branch
from
August 18, 2026 14:37
6fc6081 to
59f46b7
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
cloudstack/resource_cloudstack_instance.go:884
updateServiceOfferinglooks up the service offering ID usingretrieveServiceOfferingID(...)filtered by zone, butupdateComputeDetailsuses a genericretrieveID(...)for the current service offering. If service offering names are not globally unique (or are zone-scoped), this can select the wrong offering ID. To keep behavior consistent and deterministic, fetchzoneidand useretrieveServiceOfferingID(cs, zoneid, ...)here as well (or otherwise ensure the lookup is zone-scoped when the value is a name).
// Get current service offering ID for details-only scaling
currentServiceOfferingID, e := retrieveID(cs, "service_offering", d.Get("service_offering").(string))
if e != nil {
return e.Error()
}
p := cs.VirtualMachine.NewScaleVirtualMachineParams(d.Id(), currentServiceOfferingID)
p.SetDetails(detailsForAPI)
cloudstack/resource_cloudstack_instance.go:974
- The comment says non-string values are skipped, but the current code still allocates
strKeyPairsat full length and leaves empty-string entries for any skipped values. That can result in sending empty keypair names to the API. Prefer building the slice withappendonly for valid strings (or compacting the slice beforeSetKeypairs).
// Safely convert []interface{} to []string, skipping non-string values
strKeyPairs := make([]string, len(keypairsInterfaces))
for i, v := range keypairsInterfaces {
switch v := v.(type) {
case string:
strKeyPairs[i] = v
default:
log.Printf("Value at index %d is not a string: %v", i, v)
continue
}
}
p.SetKeypairs(strKeyPairs)
cloudstack/resource_cloudstack_instance_test.go:496
- This check doesn’t actually assert that CPU/memory changed as a result of scaling—
Cpunumber > 0andMemory > 0would typically be true even before scaling, so the test may still pass even if the original bug regresses (service offering metadata updates but compute resources don’t). To make the test verify real scaling, capture CPU/memory after the first step and assert they change (e.g., increased) after the scale-up step, or query the expected CPU/memory for the target service offering via the CloudStack API and compare against the VM’sCpunumber/Memory.
func testAccCheckCloudStackInstanceScaled(
instance *cloudstack.VirtualMachine) resource.TestCheckFunc {
return func(s *terraform.State) error {
// Verify that the VM has actually been scaled by checking CPU and memory
// are set to the Medium Instance values. This ensures ScaleVirtualMachine
// was invoked and completed successfully.
if instance.Cpunumber <= 0 {
return fmt.Errorf("CPU number not set after scaling, got: %d", instance.Cpunumber)
}
if instance.Memory <= 0 {
return fmt.Errorf("Memory not set after scaling, got: %d", instance.Memory)
}
// Medium Instance should have more resources than Small Instance
// (this is environment-dependent, but at minimum both should be > 0)
if instance.Serviceofferingname != "Medium Instance" {
return fmt.Errorf("Bad service offering after scaling: %s", instance.Serviceofferingname)
}
cloudstack/resource_cloudstack_instance.go:718
- If a plan changes both
service_offeringand compute-relateddetails, the update path can callScaleVirtualMachinetwice (once inupdateServiceOffering, once inupdateComputeDetails). Consider combining into a single scale call when both changes are present (e.g., haveupdateServiceOfferingoptionally include compute-related details), to reduce API calls and avoid longer maintenance windows while the VM is stopped.
if err := updateInstanceName(cs, d, name); err != nil {
return err
}
if err := updateServiceOffering(cs, d, name); err != nil {
return err
}
if err := updateComputeDetails(cs, d, name); err != nil {
return err
}
Comment on lines
858
to
869
| detailsForAPI[k] = fmt.Sprintf("%v", v) | ||
| } | ||
|
|
||
| if keypair, ok := d.GetOk("keypair"); ok { | ||
| p.SetKeypair(keypair.(string)) | ||
| } | ||
| // Get current service offering ID for details-only scaling | ||
| currentServiceOfferingID, e := retrieveID(cs, "service_offering", d.Get("service_offering").(string)) | ||
| if e != nil { | ||
| return e.Error() | ||
| } | ||
|
|
||
| if keypairs, ok := d.GetOk("keypairs"); ok { | ||
| p := cs.VirtualMachine.NewScaleVirtualMachineParams(d.Id(), currentServiceOfferingID) | ||
| p.SetDetails(detailsForAPI) | ||
|
|
updateComputeDetails returned early whenever no compute key (cpuNumber, cpuSpeed, memory) changed, so edits to any other detail key were silently dropped -- a regression from the prior standalone details block that always applied the full map via UpdateVirtualMachine. Restore that: scale via ScaleVirtualMachine only when a compute key changed, then always persist the full details map via UpdateVirtualMachine so custom/non-compute detail changes take effect.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (5)
cloudstack/resource_cloudstack_instance.go:698
- Including
d.HasChange("details")in the reboot-required condition will stop/start the VM for any details change, even when only non-compute metadata keys changed. Previously, details updates were applied without forcing downtime; this change can introduce unnecessary outages for users updating non-scaling details.
// Attributes that require reboot to update
if d.HasChange("name") || d.HasChange("service_offering") || d.HasChange("details") ||
d.HasChange("affinity_group_ids") || d.HasChange("affinity_group_names") ||
d.HasChange("keypair") || d.HasChange("keypairs") ||
d.HasChange("user_data") || d.HasChange("userdata_id") || d.HasChange("userdata_details") {
cloudstack/resource_cloudstack_instance.go:744
- The start-after-changes failure path drops the underlying error, which makes troubleshooting CloudStack API failures difficult.
if err != nil {
return fmt.Errorf(
"Error starting instance %s after making changes", name)
}
cloudstack/resource_cloudstack_instance.go:882
- For details-only scaling, the service offering ID lookup uses
retrieveID("service_offering", ...), which is not zone-aware. Elsewhere (create/updateServiceOffering) service offerings are resolved viaretrieveServiceOfferingIDscoped to the VM's zone to avoid ambiguity when the same offering name exists in multiple zones.
// Get current service offering ID for details-only scaling
currentServiceOfferingID, e := retrieveID(cs, "service_offering", d.Get("service_offering").(string))
if e != nil {
return e.Error()
}
cloudstack/resource_cloudstack_instance.go:813
d.GetChange("service_offering")returnsinterface{}values, but the log/error strings format them with%s. This produces%!s(...)output and can obscure what actually changed.
oldOffering, newOffering := d.GetChange("service_offering")
log.Printf("[DEBUG] Service offering changed for %s from %s to %s, starting scale", name, oldOffering, newOffering)
cloudstack/resource_cloudstack_instance_test.go:497
- Checking only that CPU/memory are
> 0does not prove ScaleVirtualMachine resized the VM; those fields are typically non-zero even when only the service offering metadata changes. The test should validate that the VM's CPU/memory match the target service offering's defined resources.
// Verify that the VM has actually been scaled by checking CPU and memory
// are set to the Medium Instance values. This ensures ScaleVirtualMachine
// was invoked and completed successfully.
if instance.Cpunumber <= 0 {
return fmt.Errorf("CPU number not set after scaling, got: %d", instance.Cpunumber)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the service_offering of an instance changes, the provider must call ScaleVirtualMachine to actually scale the VM's CPU and memory resources. The previous implementation used ChangeServiceForVirtualMachine which only updated CloudStack metadata without scaling the actual VM.
This change ensures that when a service offering is updated, the VM is properly scaled with the new compute resources.
Fixes: #273