provider/azure: fix volume detachment

When detaching the final volume from an Azure machine, we were setting the VM's data disks field to nil, rather than a pointer to an empty slice. This was apparently ignored by Azure, so the data disk remained attached. Setting the field so it is a pointer to an empty slice causes the field to be marshalled, and Azure detaches the disk.
ObviouslyGreen · Aug 28, 2017 · bbc31a4 · bbc31a4
1 parent 3aaad60
commit bbc31a4
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 5 deletions.
diff --git a/provider/azure/storage.go b/provider/azure/storage.go
@@ -564,11 +564,7 @@ func (v *azureVolumeSource) detachVolume(
 			continue
 		}
 		dataDisks = append(dataDisks[:i], dataDisks[i+1:]...)
-		if len(dataDisks) == 0 {
-			vm.StorageProfile.DataDisks = nil
-		} else {
-			*vm.StorageProfile.DataDisks = dataDisks
-		}
+		vm.StorageProfile.DataDisks = &dataDisks
 		return true
 	}
 	return false

diff --git a/provider/azure/storage_test.go b/provider/azure/storage_test.go
@@ -608,3 +608,63 @@ func (s *storageSuite) TestDetachVolumes(c *gc.C) {
 	virtualMachines[0].StorageProfile.DataDisks = &machine0DataDisks
 	assertRequestBody(c, s.requests[2], &virtualMachines[0])
 }
+
+func (s *storageSuite) TestDetachVolumesFinal(c *gc.C) {
+	// machine-0 has a one data disk: volume-0.
+	machine0DataDisks := []compute.DataDisk{{
+		Lun:  to.Int32Ptr(0),
+		Name: to.StringPtr("volume-0"),
+		Vhd: &compute.VirtualHardDisk{
+			URI: to.StringPtr(fmt.Sprintf(
+				"https://%s.blob.storage.azurestack.local/datavhds/volume-0.vhd",
+				storageAccountName,
+			)),
+		},
+	}}
+
+	params := []storage.VolumeAttachmentParams{{
+		AttachmentParams: storage.AttachmentParams{
+			Provider:   "azure",
+			Machine:    names.NewMachineTag("0"),
+			InstanceId: instance.Id("machine-0"),
+		},
+		Volume:   names.NewVolumeTag("0"),
+		VolumeId: "volume-0",
+	}}
+
+	virtualMachines := []compute.VirtualMachine{{
+		Name: to.StringPtr("machine-0"),
+		VirtualMachineProperties: &compute.VirtualMachineProperties{
+			StorageProfile: &compute.StorageProfile{DataDisks: &machine0DataDisks},
+		},
+	}}
+
+	// There should be a one API call to list VMs, and one update to the VM.
+	virtualMachinesSender := azuretesting.NewSenderWithValue(compute.VirtualMachineListResult{
+		Value: &virtualMachines,
+	})
+	virtualMachinesSender.PathPattern = `.*/Microsoft\.Compute/virtualMachines`
+	updateVirtualMachine0Sender := azuretesting.NewSenderWithValue(&compute.VirtualMachine{})
+	updateVirtualMachine0Sender.PathPattern = `.*/Microsoft\.Compute/virtualMachines/machine-0`
+	volumeSource := s.volumeSource(c)
+	s.sender = azuretesting.Senders{
+		virtualMachinesSender,
+		s.accountSender(),
+		updateVirtualMachine0Sender,
+	}
+
+	results, err := volumeSource.DetachVolumes(params)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(results, gc.HasLen, len(params))
+	c.Assert(results[0], jc.ErrorIsNil)
+
+	// Validate HTTP request bodies.
+	c.Assert(s.requests, gc.HasLen, 3)
+	c.Assert(s.requests[0].Method, gc.Equals, "GET") // list virtual machines
+	c.Assert(s.requests[1].Method, gc.Equals, "GET") // list storage accounts
+	c.Assert(s.requests[2].Method, gc.Equals, "PUT") // update machine-0
+
+	machine0DataDisks = []compute.DataDisk{}
+	virtualMachines[0].StorageProfile.DataDisks = &machine0DataDisks
+	assertRequestBody(c, s.requests[2], &virtualMachines[0])
+}