From 1baec10124e2f64285ac57de41385078653a510c Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Fri, 16 Feb 2018 15:50:34 -0800 Subject: [PATCH 1/3] r/virtual_machine: Use CapacityInKB when CapacityInBytes unavailable Under certain situations, namely vCenter upgrades, the CapacityInBytes field for virtual disks gets cleared, which will cause issues fetching settings for virtual disks on templates and virtual machines post-upgrade. This updates the virtual disk logic for both resources and data sources so that if the value in CapacityInBytes is missing (read: less than 1), we fall back to CapacityInKB, which will still be populated. --- .../virtual_machine_disk_subresource.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go b/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go index 602a5d0bf..b40791999 100644 --- a/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go +++ b/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go @@ -1094,7 +1094,7 @@ func ReadDiskAttrsForDataSource(l object.VirtualDeviceList, count int) ([]map[st if backing.ThinProvisioned != nil { thin = *backing.ThinProvisioned } - m["size"] = int(structure.ByteToGiB(disk.CapacityInBytes).(int64)) + m["size"] = diskCapacityInGiB(disk) m["eagerly_scrub"] = eager m["thin_provisioned"] = thin out = append(out, m) @@ -1194,7 +1194,7 @@ func (r *DiskSubresource) Read(l object.VirtualDeviceList) error { return fmt.Errorf("could not parse path from filename: %s", b.FileName) } r.Set("path", dp.Path) - r.Set("size", structure.ByteToGiB(disk.CapacityInBytes)) + r.Set("size", diskCapacityInGiB(disk)) } if allocation := disk.StorageIOAllocation; allocation != nil { @@ -1876,3 +1876,18 @@ func diskUUIDMatch(device types.BaseVirtualDevice, uuid string) bool { } return true } + +// diskCapacityInGiB reports the supplied disk's capacity, by first checking +// CapacityInBytes, and then falling back to CapacityInKB if that value is +// unavailable. This helps correct some situations where the former value's +// data gets cleared, which seems to happen on upgrades. +func diskCapacityInGiB(disk *types.VirtualDisk) int { + if disk.CapacityInBytes > 0 { + return int(structure.ByteToGiB(disk.CapacityInBytes).(int64)) + } + log.Printf( + "[DEBUG] diskCapacityInGiB: capacityInBytes missing for for %s, falling back to capacityInKB", + object.VirtualDeviceList{}.Name(disk), + ) + return int(structure.ByteToGiB(disk.CapacityInKB * 1024).(int64)) +} From 89689731266b587b21f3f94880da2461133722c5 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Fri, 16 Feb 2018 16:16:11 -0800 Subject: [PATCH 2/3] r/virtual_machine: Don't commit empty disk sharing value to state This is another post-upgrade thing. While vCenter will be upgraded, VM hardware versions may remain at specific versions, especially if the underlying hypervisors have not been upgraded yet. In this situation, disk sharing will be an unset value and setting it will yield an error, hence we skip setting it so that it does not create a diff. This kind of highlights the need for us to be aware of the VM hardware version in the future, in addition to the specific version of vCenter the endpoint is on. --- .../virtualdevice/virtual_machine_disk_subresource.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go b/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go index b40791999..88c0a61d1 100644 --- a/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go +++ b/vsphere/internal/virtualdevice/virtual_machine_disk_subresource.go @@ -1175,9 +1175,13 @@ func (r *DiskSubresource) Read(l object.VirtualDeviceList) error { r.Set("disk_mode", b.DiskMode) r.Set("write_through", b.WriteThrough) - // Only use disk_sharing if we are on vSphere 6.0 and higher + // Only use disk_sharing if we are on vSphere 6.0 and higher. In addition, + // skip if the value is unset - this prevents spurious diffs during upgrade + // situations where the VM hardware version does not actually allow disk + // sharing. In this situation, the value will be blank, and setting it will + // actually result in an error. version := viapi.ParseVersionFromClient(r.client) - if version.Newer(viapi.VSphereVersion{Product: version.Product, Major: 6}) { + if version.Newer(viapi.VSphereVersion{Product: version.Product, Major: 6}) && b.Sharing != "" { r.Set("disk_sharing", b.Sharing) } From c4e016a12f123948ef4ef784b782c79250cabc43 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Mon, 26 Feb 2018 08:13:53 -0800 Subject: [PATCH 3/3] r/virtual_machine: Adding test for diskCapacityInGiB --- .../virtual_machine_disk_subresource_test.go | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 vsphere/internal/virtualdevice/virtual_machine_disk_subresource_test.go diff --git a/vsphere/internal/virtualdevice/virtual_machine_disk_subresource_test.go b/vsphere/internal/virtualdevice/virtual_machine_disk_subresource_test.go new file mode 100644 index 000000000..d4661ef7c --- /dev/null +++ b/vsphere/internal/virtualdevice/virtual_machine_disk_subresource_test.go @@ -0,0 +1,39 @@ +package virtualdevice + +import ( + "testing" + + "github.com/vmware/govmomi/vim25/types" +) + +func TestDiskCapacityInGiB(t *testing.T) { + cases := []struct { + name string + subject *types.VirtualDisk + expected int + }{ + { + name: "capacityInBytes", + subject: &types.VirtualDisk{ + CapacityInBytes: 4294967296, + CapacityInKB: 4194304, + }, + expected: 4, + }, + { + name: "capacityInKB", + subject: &types.VirtualDisk{ + CapacityInKB: 4194304, + }, + expected: 4, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + actual := diskCapacityInGiB(tc.subject) + if tc.expected != actual { + t.Fatalf("expected %d, got %d", tc.expected, actual) + } + }) + } +}