Fixed default disk provisioning type & added tests

* Fixed the default disk provisioning type if the model does not hold a valid option * Added tests
juju · jujubot · Apr 21, 2021 · Mar 26, 2021 · Mar 26, 2021 · Mar 26, 2021
commit be6120ca7913d3558ce40fe7536ee932fa520c90
diff --git a/provider/vsphere/config.go b/provider/vsphere/config.go
@@ -126,9 +126,17 @@ func (c *environConfig) forceVMHardwareVersion() int64 {
 func (c *environConfig) diskProvisioningType() vsphereclient.DiskProvisioningType {
 	provType, ok := c.attrs[cfgDiskProvisioningType]
 	if !ok {
-		return vsphereclient.DiskTypeThin
+		// Return the default in case none is set.
+		return vsphereclient.DefaultDiskProvisioningType
 	}
-	return vsphereclient.DiskProvisioningType(provType.(string))
+
+	provTypeStr, ok := provType.(string)
+	if !ok || provTypeStr == "" {
+		// We got an invalid value set, return default.
+		return vsphereclient.DefaultDiskProvisioningType
+	}
+
+	return vsphereclient.DiskProvisioningType(provTypeStr)
 }
 
 // validate checks vmware-specific config values.
@@ -141,22 +149,23 @@ func (c environConfig) validate() error {
 	}
 
 	if diskProvType, ok := c.attrs[cfgDiskProvisioningType]; ok {
-
 		diskProvTypeStr, ok := diskProvType.(string)
 		if !ok {
 			return errors.Errorf("%s must be a string", cfgDiskProvisioningType)
 		}
 
-		found := false
-		for _, val := range vsphereclient.ValidDiskProvisioningTypes {
-			if vsphereclient.DiskProvisioningType(diskProvTypeStr) == val {
-				found = true
-				break
+		if diskProvTypeStr != "" {
+			found := false
+			for _, val := range vsphereclient.ValidDiskProvisioningTypes {
+				if vsphereclient.DiskProvisioningType(diskProvTypeStr) == val {
+					found = true
+					break
+				}
+			}
+			if !found {
+				return errors.Errorf(
+					"%q must be one of %q", cfgDiskProvisioningType, vsphereclient.ValidDiskProvisioningTypes)
 			}
-		}
-		if !found {
-			return errors.Errorf(
-				"%q must be one of %q", cfgDiskProvisioningType, vsphereclient.ValidDiskProvisioningTypes)
 		}
 	}
 	return nil

diff --git a/provider/vsphere/config_test.go b/provider/vsphere/config_test.go
@@ -28,7 +28,7 @@ func fakeConfigAttrs(attrs ...testing.Attrs) testing.Attrs {
 		"external-network":          "",
 		"enable-disk-uuid":          true,
 		"force-vm-hardware-version": 0,
-		"disk-provisioning-type":    "thin",
+		"disk-provisioning-type":    "",
 	})
 	for _, attrs := range attrs {
 		merged = merged.Merge(attrs)

diff --git a/provider/vsphere/environ_broker_test.go b/provider/vsphere/environ_broker_test.go
@@ -151,7 +151,7 @@ func (s *legacyEnvironBrokerSuite) TestStartInstance(c *gc.C) {
 		UpdateProgressInterval: 5 * time.Second,
 		EnableDiskUUID:         true,
 		IsBootstrap:            true,
-		DiskProvisioningType:   vsphereclient.DiskTypeThin,
+		DiskProvisioningType:   vsphereclient.DefaultDiskProvisioningType,
 	})
 
 	ovaLocation, ovaReadCloser, err := readOVA()
@@ -188,6 +188,100 @@ func (s *legacyEnvironBrokerSuite) TestStartInstanceNetwork(c *gc.C) {
 	c.Assert(createVMArgs.NetworkDevices[1].Network, gc.Equals, "bar")
 }
 
+func (s *legacyEnvironBrokerSuite) TestStartInstanceDiskProvisioningMissingModelConfigOption(c *gc.C) {
+	env, err := s.provider.Open(environs.OpenParams{
+		Cloud: fakeCloudSpec(),
+		Config: fakeConfig(c, coretesting.Attrs{
+			"image-metadata-url": s.imageServer.URL,
+		}),
+	})
+	c.Assert(err, jc.ErrorIsNil)
+
+	result, err := env.StartInstance(s.callCtx, s.createStartInstanceArgs(c))
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result, gc.NotNil)
+
+	call := s.client.Calls()[4]
+	createVMArgs := call.Args[1].(vsphereclient.CreateVirtualMachineParams)
+	c.Assert(createVMArgs.DiskProvisioningType, gc.Equals, vsphereclient.DefaultDiskProvisioningType)
+}
+
+func (s *legacyEnvironBrokerSuite) TestStartInstanceDiskProvisioningDefaultOption(c *gc.C) {
+	env, err := s.provider.Open(environs.OpenParams{
+		Cloud: fakeCloudSpec(),
+		Config: fakeConfig(c, coretesting.Attrs{
+			"image-metadata-url":     s.imageServer.URL,
+			"disk-provisioning-type": "",
+		}),
+	})
+	c.Assert(err, jc.ErrorIsNil)
+
+	result, err := env.StartInstance(s.callCtx, s.createStartInstanceArgs(c))
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result, gc.NotNil)
+
+	call := s.client.Calls()[4]
+	createVMArgs := call.Args[1].(vsphereclient.CreateVirtualMachineParams)
+	c.Assert(createVMArgs.DiskProvisioningType, gc.Equals, vsphereclient.DefaultDiskProvisioningType)
+}
+
+func (s *legacyEnvironBrokerSuite) TestStartInstanceDiskProvisioningThinDisk(c *gc.C) {
+	env, err := s.provider.Open(environs.OpenParams{
+		Cloud: fakeCloudSpec(),
+		Config: fakeConfig(c, coretesting.Attrs{
+			"image-metadata-url":     s.imageServer.URL,
+			"disk-provisioning-type": "thin",
+		}),
+	})
+	c.Assert(err, jc.ErrorIsNil)
+
+	result, err := env.StartInstance(s.callCtx, s.createStartInstanceArgs(c))
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result, gc.NotNil)
+
+	call := s.client.Calls()[4]
+	createVMArgs := call.Args[1].(vsphereclient.CreateVirtualMachineParams)
+	c.Assert(createVMArgs.DiskProvisioningType, gc.Equals, vsphereclient.DiskTypeThin)
+}
+
+func (s *legacyEnvironBrokerSuite) TestStartInstanceDiskProvisioningThickDisk(c *gc.C) {
+	env, err := s.provider.Open(environs.OpenParams{
+		Cloud: fakeCloudSpec(),
+		Config: fakeConfig(c, coretesting.Attrs{
+			"image-metadata-url":     s.imageServer.URL,
+			"disk-provisioning-type": "thick",
+		}),
+	})
+	c.Assert(err, jc.ErrorIsNil)
+
+	result, err := env.StartInstance(s.callCtx, s.createStartInstanceArgs(c))
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result, gc.NotNil)
+
+	call := s.client.Calls()[4]
+	createVMArgs := call.Args[1].(vsphereclient.CreateVirtualMachineParams)
+	c.Assert(createVMArgs.DiskProvisioningType, gc.Equals, vsphereclient.DiskTypeThick)
+}
+
+func (s *legacyEnvironBrokerSuite) TestStartInstanceDiskProvisioningThickEagerZeroDisk(c *gc.C) {
+	env, err := s.provider.Open(environs.OpenParams{
+		Cloud: fakeCloudSpec(),
+		Config: fakeConfig(c, coretesting.Attrs{
+			"image-metadata-url":     s.imageServer.URL,
+			"disk-provisioning-type": "thickEagerZero",
+		}),
+	})
+	c.Assert(err, jc.ErrorIsNil)
+
+	result, err := env.StartInstance(s.callCtx, s.createStartInstanceArgs(c))
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(result, gc.NotNil)
+
+	call := s.client.Calls()[4]
+	createVMArgs := call.Args[1].(vsphereclient.CreateVirtualMachineParams)
+	c.Assert(createVMArgs.DiskProvisioningType, gc.Equals, vsphereclient.DiskTypeThickEagerZero)
+}
+
 func (s *legacyEnvironBrokerSuite) TestStartInstanceLongModelName(c *gc.C) {
 	env, err := s.provider.Open(environs.OpenParams{
 		Cloud: fakeCloudSpec(),

diff --git a/provider/vsphere/internal/vsphereclient/client.go b/provider/vsphere/internal/vsphereclient/client.go
@@ -52,7 +52,7 @@ const (
 	// DiskTypeThickEagerZero sets the provisioning type for disks to
 	// "thick eager zeroed". The entire size of the virtual disk will be
 	// deducted from the underlying datastore. Any unwritten disk areas
-	// will be zeroed out during cloning. This is the default setting.
+	// will be zeroed out during cloning.
 	DiskTypeThickEagerZero DiskProvisioningType = "thickEagerZero"
 )
 
@@ -63,6 +63,9 @@ var (
 		DiskTypeThick,
 		DiskTypeThickEagerZero,
 	}
+	// DefaultDiskProvisioningType is the default disk provisioning type
+	// if none is selected in the model config.
+	DefaultDiskProvisioningType = DiskTypeThickEagerZero
 )
 
 // ErrExtendDisk is returned if we timed out trying to extend the root

diff --git a/provider/vsphere/internal/vsphereclient/createvm_test.go b/provider/vsphere/internal/vsphereclient/createvm_test.go
@@ -183,12 +183,12 @@ func (s *clientSuite) TestCreateVirtualMachineForceHWVersion(c *gc.C) {
 	_, err := client.CreateVirtualMachine(context.Background(), args)
 	c.Assert(err, jc.ErrorIsNil)
 
-	s.roundTripper.CheckCall(c, 41, "RetrieveProperties", "FakeVm1")
-	s.roundTripper.CheckCall(c, 42, "QueryConfigOption", "FakeEnvironmentBrowser")
+	s.roundTripper.CheckCall(c, 42, "RetrieveProperties", "FakeVm1")
+	s.roundTripper.CheckCall(c, 43, "QueryConfigOption", "FakeEnvironmentBrowser")
 	// Mock server max version is vmx-13
 	// Mock template VM version is vmx-10
 	// We requested vmx-11. This should match the call to UpgradeVM_Task.
-	s.roundTripper.CheckCall(c, 43, "UpgradeVM_Task", "vmx-11")
+	s.roundTripper.CheckCall(c, 44, "UpgradeVM_Task", "vmx-11")
 }
 
 func (s *clientSuite) TestCreateVirtualMachineNoDiskUUID(c *gc.C) {