From 6cf84f819e05aaee5503fe8e0bb16feb999de86c Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Wed, 14 Apr 2021 17:19:37 +0200 Subject: [PATCH] vSphere: Return useful errors from parameter validation Returning "VolumeOptions verification failed" when StorageClass parameter validation failed is not really useful. It does not tell which parameter is wrong and what is a VolumeOption at all. Trying to return something more useful here. --- .../vsphere/vclib/custom_errors.go | 22 +++++++++---------- .../vsphere/vclib/diskmanagers/virtualdisk.go | 6 ++--- .../vsphere/vclib/volumeoptions.go | 9 ++++---- 3 files changed, 18 insertions(+), 19 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/custom_errors.go b/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/custom_errors.go index 6709c4cf21a..802901ef5aa 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/custom_errors.go +++ b/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/custom_errors.go @@ -20,20 +20,18 @@ import "errors" // Error Messages const ( - FileAlreadyExistErrMsg = "File requested already exist" - NoDiskUUIDFoundErrMsg = "No disk UUID found" - NoDevicesFoundErrMsg = "No devices found" - DiskNotFoundErrMsg = "No vSphere disk ID found" - InvalidVolumeOptionsErrMsg = "VolumeOptions verification failed" - NoVMFoundErrMsg = "No VM found" + FileAlreadyExistErrMsg = "File requested already exist" + NoDiskUUIDFoundErrMsg = "No disk UUID found" + NoDevicesFoundErrMsg = "No devices found" + DiskNotFoundErrMsg = "No vSphere disk ID found" + NoVMFoundErrMsg = "No VM found" ) // Error constants var ( - ErrFileAlreadyExist = errors.New(FileAlreadyExistErrMsg) - ErrNoDiskUUIDFound = errors.New(NoDiskUUIDFoundErrMsg) - ErrNoDevicesFound = errors.New(NoDevicesFoundErrMsg) - ErrNoDiskIDFound = errors.New(DiskNotFoundErrMsg) - ErrInvalidVolumeOptions = errors.New(InvalidVolumeOptionsErrMsg) - ErrNoVMFound = errors.New(NoVMFoundErrMsg) + ErrFileAlreadyExist = errors.New(FileAlreadyExistErrMsg) + ErrNoDiskUUIDFound = errors.New(NoDiskUUIDFoundErrMsg) + ErrNoDevicesFound = errors.New(NoDevicesFoundErrMsg) + ErrNoDiskIDFound = errors.New(DiskNotFoundErrMsg) + ErrNoVMFound = errors.New(NoVMFoundErrMsg) ) diff --git a/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/diskmanagers/virtualdisk.go b/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/diskmanagers/virtualdisk.go index 3bb9af0fab4..88b50641a5c 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/diskmanagers/virtualdisk.go +++ b/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/diskmanagers/virtualdisk.go @@ -64,9 +64,9 @@ func (virtualDisk *VirtualDisk) Create(ctx context.Context, datastore *vclib.Dat if virtualDisk.VolumeOptions.DiskFormat == "" { virtualDisk.VolumeOptions.DiskFormat = vclib.ThinDiskType } - if !virtualDisk.VolumeOptions.VerifyVolumeOptions() { - klog.Error("VolumeOptions verification failed. volumeOptions: ", virtualDisk.VolumeOptions) - return "", vclib.ErrInvalidVolumeOptions + if err := virtualDisk.VolumeOptions.VerifyVolumeOptions(); err != nil { + klog.Errorf("VolumeOptions verification failed: %s (options: %+v)", err, virtualDisk.VolumeOptions) + return "", fmt.Errorf("validation of parameters failed: %s", err) } if virtualDisk.VolumeOptions.StoragePolicyID != "" && virtualDisk.VolumeOptions.StoragePolicyName != "" { return "", fmt.Errorf("Storage Policy ID and Storage Policy Name both set, Please set only one parameter") diff --git a/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/volumeoptions.go b/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/volumeoptions.go index 6ff0ad58aec..ca9be55bed1 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/volumeoptions.go +++ b/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/volumeoptions.go @@ -17,6 +17,7 @@ limitations under the License. package vclib import ( + "fmt" "strings" "k8s.io/api/core/v1" @@ -90,21 +91,21 @@ func CheckControllerSupported(ctrlType string) bool { } // VerifyVolumeOptions checks if volumeOptions.SCIControllerType is valid controller type -func (volumeOptions VolumeOptions) VerifyVolumeOptions() bool { +func (volumeOptions VolumeOptions) VerifyVolumeOptions() error { // Validate only if SCSIControllerType is set by user. // Default value is set later in virtualDiskManager.Create and vmDiskManager.Create if volumeOptions.SCSIControllerType != "" { isValid := CheckControllerSupported(volumeOptions.SCSIControllerType) if !isValid { - return false + return fmt.Errorf("invalid scsiControllerType: %s", volumeOptions.SCSIControllerType) } } // ThinDiskType is the default, so skip the validation. if volumeOptions.DiskFormat != ThinDiskType { isValid := CheckDiskFormatSupported(volumeOptions.DiskFormat) if !isValid { - return false + return fmt.Errorf("invalid diskFormat: %s", volumeOptions.DiskFormat) } } - return true + return nil }