diff --git a/cloudstack/resource_cloudstack_disk.go b/cloudstack/resource_cloudstack_disk.go index 8a1c5529..988ebb33 100644 --- a/cloudstack/resource_cloudstack_disk.go +++ b/cloudstack/resource_cloudstack_disk.go @@ -268,7 +268,7 @@ func resourceCloudStackDiskUpdate(d *schema.ResourceData, meta interface{}) erro // If the device ID changed, just detach here so we can re-attach the // volume at the end of this function - if d.HasChange("device_id") || d.HasChange("virtual_machine") { + if d.HasChange("device_id") || d.HasChange("virtual_machine_id") { // Detach the volume if err := resourceCloudStackDiskDetach(d, meta); err != nil { return fmt.Errorf("Error detaching disk %s from virtual machine: %s", name, err) @@ -385,9 +385,19 @@ func resourceCloudStackDiskDetach(d *schema.ResourceData, meta interface{}) erro // Detach the currently attached volume _, err := cs.Volume.DetachVolume(p) if err != nil { - if virtualmachineid, ok := d.GetOk("virtual_machine_id"); ok { + // The disk is on the VM currently holding it, which is the old value + // while virtual_machine_id is being changed. + vmid := "" + if d.HasChange("virtual_machine_id") { + oldVM, _ := d.GetChange("virtual_machine_id") + vmid = oldVM.(string) + } else if v, ok := d.GetOk("virtual_machine_id"); ok { + vmid = v.(string) + } + + if vmid != "" { // Create a new parameter struct - pd := cs.VirtualMachine.NewStopVirtualMachineParams(virtualmachineid.(string)) + pd := cs.VirtualMachine.NewStopVirtualMachineParams(vmid) // Stop the virtual machine in order to be able to detach the disk if _, err := cs.VirtualMachine.StopVirtualMachine(pd); err != nil { @@ -400,7 +410,7 @@ func resourceCloudStackDiskDetach(d *schema.ResourceData, meta interface{}) erro } // Create a new parameter struct - pu := cs.VirtualMachine.NewStartVirtualMachineParams(virtualmachineid.(string)) + pu := cs.VirtualMachine.NewStartVirtualMachineParams(vmid) // Start the virtual machine again if _, err := cs.VirtualMachine.StartVirtualMachine(pu); err != nil { diff --git a/cloudstack/resource_cloudstack_disk_unit_test.go b/cloudstack/resource_cloudstack_disk_unit_test.go new file mode 100644 index 00000000..d2823e16 --- /dev/null +++ b/cloudstack/resource_cloudstack_disk_unit_test.go @@ -0,0 +1,102 @@ +// +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +// + +package cloudstack + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/apache/cloudstack-go/v2/cloudstack" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" +) + +func diskUpdateData(t *testing.T) *schema.ResourceData { + sm := schema.InternalMap(resourceCloudStackDisk().Schema) + state := &terraform.InstanceState{ + ID: "vol-1", + Attributes: map[string]string{ + "name": "disk-1", + "virtual_machine_id": "vm-1", + "attach": "true", + }, + } + diff := &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "virtual_machine_id": {Old: "vm-1", New: "vm-2"}, + }, + } + d, err := sm.Data(state, diff) + if err != nil { + t.Fatalf("building resource data: %s", err) + } + return d +} + +// The disk's attached VM is stored under virtual_machine_id, but the update +// detected a VM change with HasChange("virtual_machine"), a key that does not +// exist, so the branch never ran. A virtual_machine_id change must reach the +// detach path (rather than falling through to the attach path). +func TestDiskUpdateDetachesWhenVirtualMachineChanges(t *testing.T) { + cs := cloudstack.NewClient("http://127.0.0.1:1", "key", "secret", false) + + if err := resourceCloudStackDiskUpdate(diskUpdateData(t), cs); err == nil { + t.Fatal("expected an error, got nil") + } else if !strings.Contains(err.Error(), "detaching") { + t.Fatalf("a virtual_machine_id change should trigger a detach; got: %s", err) + } +} + +// When a plain detach fails, the fallback stops the VM currently holding the +// disk before retrying. During a virtual_machine_id change that VM is the old +// value, not the new one, so the fallback must stop the source VM. +func TestDiskUpdateDetachFallbackStopsSourceVM(t *testing.T) { + var stoppedVM string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + r.ParseForm() + switch r.Form.Get("command") { + case "listVolumes": + // The volume is still attached to the old VM. + w.Write([]byte(`{"listvolumesresponse":{"count":1,"volume":[{"id":"vol-1","virtualmachineid":"vm-1"}]}}`)) + case "detachVolume": + // Fail so the stop/retry fallback runs. + w.WriteHeader(http.StatusExpectationFailed) + w.Write([]byte(`{"detachvolumeresponse":{"errorcode":431,"errortext":"cannot detach"}}`)) + case "stopVirtualMachine": + stoppedVM = r.Form.Get("id") + w.WriteHeader(http.StatusExpectationFailed) + w.Write([]byte(`{"stopvirtualmachineresponse":{"errorcode":431,"errortext":"stop failed"}}`)) + default: + w.Write([]byte(`{"queryasyncjobresultresponse":{"jobstatus":2,"jobresult":{"errorcode":431,"errortext":"failed"}}}`)) + } + })) + defer server.Close() + + cs := cloudstack.NewClient(server.URL, "key", "secret", false) + + _ = resourceCloudStackDiskUpdate(diskUpdateData(t), cs) + + if stoppedVM != "vm-1" { + t.Fatalf("detach fallback should stop the source VM vm-1, stopped %q", stoppedVM) + } +}