Repository: vmware/govmomi
Pull request #2895 (2022-07-06 14:28:08)

vcsim: Fix datastore freespace changed by ReconfigVM_Task
## Description

This fixes datastore freespace changing logic in `ReconfigVM_Task`.
When virtualDisk's size is changed, datastore freespace decreases by disk capacity difference.

Closes: #2894

## Type of change

Please mark options that are relevant:

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
- [ ] This change requires a documentation update
- [ ] Build related change

## How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. If applicable, please also list any relevant
details for your test configuration.

- [x] add unit tests

## Checklist:

- [x] My code follows the `CONTRIBUTION`
  [guidelines](https://github.com/vmware/govmomi/blob/master/CONTRIBUTING.md) of
  this project
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged


Patch:
diff --git a/simulator/virtual_machine.go b/simulator/virtual_machine.go
index 49fd037e..c72da1c4 100644
--- a/simulator/virtual_machine.go
+++ b/simulator/virtual_machine.go
@@ -1106,7 +1106,12 @@ func (vm *VirtualMachine) validateSwitchMembers(id string) types.BaseMethodFault
 	return nil
 }
 
-func (vm *VirtualMachine) configureDevice(ctx *Context, devices object.VirtualDeviceList, spec *types.VirtualDeviceConfigSpec) types.BaseMethodFault {
+func (vm *VirtualMachine) configureDevice(
+	ctx *Context,
+	devices object.VirtualDeviceList,
+	spec *types.VirtualDeviceConfigSpec,
+	oldDevice types.BaseVirtualDevice) types.BaseMethodFault {
+
 	device := spec.Device
 	d := device.GetVirtualDevice()
 	var controller types.BaseVirtualController
@@ -1220,6 +1225,17 @@ func (vm *VirtualMachine) configureDevice(ctx *Context, devices object.VirtualDe
 			ds := vm.findDatastore(p.Datastore)
 			info.Datastore = &ds.Self
 
+			if oldDevice != nil {
+				if oldDisk, ok := oldDevice.(*types.VirtualDisk); ok {
+					// add previous capacity to datastore freespace
+					ctx.WithLock(ds, func() {
+						ds.Summary.FreeSpace += getDiskSize(oldDisk)
+						ds.Info.GetDatastoreInfo().FreeSpace = ds.Summary.FreeSpace
+					})
+				}
+			}
+
+			// then subtract new capacity from datastore freespace
 			// XXX: compare disk size and free space until windows stat is supported
 			ctx.WithLock(ds, func() {
 				ds.Summary.FreeSpace -= getDiskSize(x)
@@ -1453,7 +1469,7 @@ func (vm *VirtualMachine) configureDevices(ctx *Context, spec *types.VirtualMach
 			}
 
 			key := device.Key
-			err := vm.configureDevice(ctx, devices, dspec)
+			err := vm.configureDevice(ctx, devices, dspec, nil)
 			if err != nil {
 				return err
 			}
@@ -1470,16 +1486,17 @@ func (vm *VirtualMachine) configureDevices(ctx *Context, spec *types.VirtualMach
 			}
 		case types.VirtualDeviceConfigSpecOperationEdit:
 			rspec := *dspec
-			rspec.Device = devices.FindByKey(device.Key)
-			if rspec.Device == nil {
+			oldDevice := devices.FindByKey(device.Key)
+			if oldDevice == nil {
 				return invalid
 			}
+			rspec.Device = oldDevice
 			devices = vm.removeDevice(ctx, devices, &rspec)
 			if device.DeviceInfo != nil {
 				device.DeviceInfo.GetDescription().Summary = "" // regenerate summary
 			}
 
-			err := vm.configureDevice(ctx, devices, dspec)
+			err := vm.configureDevice(ctx, devices, dspec, oldDevice)
 			if err != nil {
 				return err
 			}


Test patch:
diff --git a/simulator/virtual_machine_test.go b/simulator/virtual_machine_test.go
index 3b1c9fb9..23d7df65 100644
--- a/simulator/virtual_machine_test.go
+++ b/simulator/virtual_machine_test.go
@@ -1302,6 +1302,114 @@ func TestReconfigureDevicesCapacity(t *testing.T) {
 	}
 }
 
+func TestReconfigureDevicesDatastoreFreespace(t *testing.T) {
+	tests := []struct {
+		name          string
+		reconfigure   func(context.Context, *object.VirtualMachine, *Datastore, object.VirtualDeviceList) error
+		freespaceDiff int64
+	}{
+		{
+			"create a new disk",
+			func(ctx context.Context, vm *object.VirtualMachine, ds *Datastore, l object.VirtualDeviceList) error {
+				controller, err := l.FindDiskController("")
+				if err != nil {
+					return err
+				}
+
+				disk := l.CreateDisk(controller, ds.Reference(), "")
+				disk.CapacityInBytes = 10 * 1024 * 1024 * 1024 // 10GB
+
+				if err := vm.AddDevice(ctx, disk); err != nil {
+					return err
+				}
+				return nil
+			},
+			-10 * 1024 * 1024 * 1024, // -10GB
+		},
+		{
+			"edit disk size",
+			func(ctx context.Context, vm *object.VirtualMachine, ds *Datastore, l object.VirtualDeviceList) error {
+				disks := l.SelectByType((*types.VirtualDisk)(nil))
+				if len(disks) == 0 {
+					return fmt.Errorf("disk not found")
+				}
+				disk := disks[len(disks)-1].(*types.VirtualDisk)
+
+				// specify same disk capacity
+				if err := vm.EditDevice(ctx, disk); err != nil {
+					return err
+				}
+				return nil
+			},
+			0,
+		},
+		{
+			"remove a disk and its files",
+			func(ctx context.Context, vm *object.VirtualMachine, ds *Datastore, l object.VirtualDeviceList) error {
+				disks := l.SelectByType((*types.VirtualDisk)(nil))
+				if len(disks) == 0 {
+					return fmt.Errorf("disk not found")
+				}
+				disk := disks[len(disks)-1].(*types.VirtualDisk)
+
+				if err := vm.RemoveDevice(ctx, false, disk); err != nil {
+					return err
+				}
+				return nil
+			},
+			10 * 1024 * 1024 * 1024, // 10GB
+		},
+		{
+			"remove a disk but keep its files",
+			func(ctx context.Context, vm *object.VirtualMachine, ds *Datastore, l object.VirtualDeviceList) error {
+				disks := l.SelectByType((*types.VirtualDisk)(nil))
+				if len(disks) == 0 {
+					return fmt.Errorf("disk not found")
+				}
+				disk := disks[len(disks)-1].(*types.VirtualDisk)
+
+				if err := vm.RemoveDevice(ctx, true, disk); err != nil {
+					return err
+				}
+				return nil
+			},
+			0,
+		},
+	}
+
+	for _, test := range tests {
+		test := test // assign to local var since loop var is reused
+		t.Run(test.name, func(t *testing.T) {
+			m := ESX()
+
+			Test(func(ctx context.Context, c *vim25.Client) {
+				vmm := Map.Any("VirtualMachine").(*VirtualMachine)
+				vm := object.NewVirtualMachine(c, vmm.Reference())
+
+				ds := Map.Any("Datastore").(*Datastore)
+				freespaceBefore := ds.Datastore.Summary.FreeSpace
+
+				devices, err := vm.Device(ctx)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				err = test.reconfigure(ctx, vm, ds, devices)
+				if err != nil {
+					t.Fatal(err)
+				}
+
+				freespaceAfter := ds.Datastore.Summary.FreeSpace
+
+				if freespaceAfter-freespaceBefore != test.freespaceDiff {
+					t.Errorf("difference of freespace expected %d, got %d",
+						test.freespaceDiff, freespaceAfter-freespaceBefore)
+				}
+			}, m)
+		})
+	}
+}
+
 func TestShutdownGuest(t *testing.T) {
 	// use the default vm for testing
 	ctx := context.Background()