This repository has been archived by the owner on Sep 7, 2022. It is now read-only.
SPBM policy integration for dynamic volume provisioning inside kubernetes #142
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| kind: StorageClass | ||
| apiVersion: storage.k8s.io/v1beta1 | ||
| metadata: | ||
| name: fast | ||
| provisioner: kubernetes.io/vsphere-volume | ||
| parameters: | ||
| diskformat: zeroedthick | ||
| storagePolicyName: gold | ||
| datastore: VSANDatastore |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| kind: StorageClass | ||
| apiVersion: storage.k8s.io/v1beta1 | ||
| metadata: | ||
| name: fast | ||
| provisioner: kubernetes.io/vsphere-volume | ||
| parameters: | ||
| diskformat: zeroedthick | ||
| storagePolicyName: gold |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ import ( | |
| "github.com/vmware/govmomi/vim25/types" | ||
| "golang.org/x/net/context" | ||
|
|
||
| pbm "github.com/vmware/govmomi/pbm" | ||
| k8stypes "k8s.io/apimachinery/pkg/types" | ||
| k8runtime "k8s.io/apimachinery/pkg/util/runtime" | ||
| "k8s.io/kubernetes/pkg/api/v1" | ||
|
|
@@ -165,7 +166,7 @@ type VSphereConfig struct { | |
| type Volumes interface { | ||
| // AttachDisk attaches given disk to given node. Current node | ||
| // is used when nodeName is empty string. | ||
| AttachDisk(vmDiskPath string, storagePolicyID string, nodeName k8stypes.NodeName) (diskID string, diskUUID string, err error) | ||
|
|
||
| // DetachDisk detaches given disk to given node. Current node | ||
| // is used when nodeName is empty string. | ||
|
|
@@ -189,12 +190,14 @@ type Volumes interface { | |
|
|
||
| // VolumeOptions specifies capacity, tags, name and diskFormat for a volume. | ||
| type VolumeOptions struct { | ||
| CapacityKB int | ||
| Tags map[string]string | ||
| Name string | ||
| DiskFormat string | ||
| Datastore string | ||
| VSANStorageProfileData string | ||
| StoragePolicyName string | ||
| StoragePolicyID string | ||
| } | ||
|
|
||
| // Generates Valid Options for Diskformat | ||
|
|
@@ -554,14 +557,12 @@ func (vs *VSphere) NodeAddresses(nodeName k8stypes.NodeName) ([]v1.NodeAddress, | |
| addressType = v1.NodeInternalIP | ||
| } | ||
| for _, ip := range v.IpAddress { | ||
| v1helper.AddToNodeAddresses(&addrs, | ||
| v1.NodeAddress{ | ||
| Type: addressType, | ||
| Address: ip, | ||
| }, | ||
| ) | ||
| } | ||
| } | ||
| return addrs, nil | ||
|
|
@@ -737,7 +738,7 @@ func cleanUpController(ctx context.Context, newSCSIController types.BaseVirtualD | |
| } | ||
|
|
||
| // Attaches given virtual disk volume to the compute running kubelet. | ||
| func (vs *VSphere) AttachDisk(vmDiskPath string, storagePolicyID string, nodeName k8stypes.NodeName) (diskID string, diskUUID string, err error) { | ||
| var newSCSIController types.BaseVirtualDevice | ||
|
|
||
| // Create context | ||
|
|
@@ -785,14 +786,8 @@ func (vs *VSphere) AttachDisk(vmDiskPath string, nodeName k8stypes.NodeName) (di | |
| return "", "", err | ||
| } | ||
|
|
||
| // Get VM device list | ||
| _, vmDevices, _, err := getVirtualMachineDevices(ctx, vs.cfg, vs.client, vSphereInstance) | ||
| if err != nil { | ||
| glog.Errorf("cannot get vmDevices for VM err=%s", err) | ||
| return "", "", fmt.Errorf("cannot get vmDevices for VM err=%s", err) | ||
|
|
@@ -811,9 +806,9 @@ func (vs *VSphere) AttachDisk(vmDiskPath string, nodeName k8stypes.NodeName) (di | |
|
|
||
| // Create a new finder | ||
| f := find.NewFinder(vs.client.Client, true) | ||
| // Set data center | ||
| f.SetDatacenter(dc) | ||
|
|
||
| datastorePathObj := new(object.DatastorePath) | ||
| isSuccess := datastorePathObj.FromString(vmDiskPath) | ||
| if !isSuccess { | ||
|
|
@@ -837,28 +832,54 @@ func (vs *VSphere) AttachDisk(vmDiskPath string, nodeName k8stypes.NodeName) (di | |
| backing := disk.Backing.(*types.VirtualDiskFlatVer2BackingInfo) | ||
| backing.DiskMode = string(types.VirtualDiskModeIndependent_persistent) | ||
|
|
||
| virtualMachineConfigSpec := types.VirtualMachineConfigSpec{} | ||
| deviceConfigSpec := &types.VirtualDeviceConfigSpec{ | ||
| Device: disk, | ||
| Operation: types.VirtualDeviceConfigSpecOperationAdd, | ||
| } | ||
| // Configure the disk with the SPBM profile only if ProfileID is not empty. | ||
| if storagePolicyID != "" { | ||
| profileSpec := &types.VirtualMachineDefinedProfileSpec{ | ||
| ProfileId: storagePolicyID, | ||
| } | ||
| deviceConfigSpec.Profile = append(deviceConfigSpec.Profile, profileSpec) | ||
| } | ||
| virtualMachineConfigSpec.DeviceChange = append(virtualMachineConfigSpec.DeviceChange, deviceConfigSpec) | ||
| task, err := vm.Reconfigure(ctx, virtualMachineConfigSpec) | ||
|
There was a problem hiding this comment. Do we want add check for nil task to avoid NPE? we have observed nil task when user does not have read permission. |
||
| if err != nil { | ||
| glog.Errorf("Failed to attach the disk with storagePolicy: %+q with err - %v", storagePolicyID, err) | ||
| if newSCSICreated { | ||
| cleanUpController(ctx, newSCSIController, vmDevices, vm) | ||
| } | ||
| return "", "", err | ||
| } | ||
| err = task.Wait(ctx) | ||
| if err != nil { | ||
| glog.Errorf("Failed to attach the disk with storagePolicy: %+q with err - %v", storagePolicyID, err) | ||
| if newSCSICreated { | ||
| cleanUpController(ctx, newSCSIController, vmDevices, vm) | ||
| } | ||
| return "", "", err | ||
| } | ||
|
|
||
| deviceName, diskUUID, err := getVMDiskInfo(ctx, vm, disk) | ||
| if err != nil { | ||
| if newSCSICreated { | ||
| cleanUpController(ctx, newSCSIController, vmDevices, vm) | ||
| } | ||
| vs.DetachDisk(deviceName, nodeName) | ||
| return "", "", err | ||
| } | ||
| return deviceName, diskUUID, nil | ||
| } | ||
|
|
||
| func getVMDiskInfo(ctx context.Context, vm *object.VirtualMachine, disk *types.VirtualDisk) (string, string, error) { | ||
| vmDevices, err := vm.Device(ctx) | ||
| if err != nil { | ||
| return "", "", err | ||
| } | ||
| devices := vmDevices.SelectByType(disk) | ||
| if len(devices) < 1 { | ||
| return "", "", ErrNoDevicesFound | ||
| } | ||
|
|
||
|
|
@@ -867,18 +888,13 @@ func (vs *VSphere) AttachDisk(vmDiskPath string, nodeName k8stypes.NodeName) (di | |
| deviceName := devices.Name(newDevice) | ||
|
|
||
| // get device uuid | ||
| diskUUID, err := getVirtualDiskUUID(newDevice) | ||
| if err != nil { | ||
| return "", "", err | ||
| } | ||
|
|
||
| return deviceName, diskUUID, nil | ||
| } | ||
| func getNextUnitNumber(devices object.VirtualDeviceList, c types.BaseVirtualController) (int32, error) { | ||
| // get next available SCSI controller unit number | ||
| var takenUnitNumbers [SCSIDeviceSlots]bool | ||
|
|
@@ -1266,19 +1282,43 @@ func (vs *VSphere) CreateVolume(volumeOptions *VolumeOptions) (volumePath string | |
| dc, err := f.Datacenter(ctx, vs.cfg.Global.Datacenter) | ||
| f.SetDatacenter(dc) | ||
|
|
||
| if volumeOptions.StoragePolicyName != "" { | ||
|
There was a problem hiding this comment. Shouldn't you fail early if storagePolicyName and VSANStorageProfileData are set? If we are already doing it, can you point to the code where we fail early? There was a problem hiding this comment. You can see that code in vspherE_volume_util.go file - 6d41087#diff-482fa05b285b59ab0bc70641f984ce98R131 |
||
| // Get the pbm client | ||
| pbmClient, err := pbm.NewClient(ctx, vs.client.Client) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| volumeOptions.StoragePolicyID, err = pbmClient.ProfileIDByName(ctx, volumeOptions.StoragePolicyName) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| // Get the resource pool for current node. | ||
| resourcePool, err := vs.getCurrentNodeResourcePool(ctx, dc) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| dsRefs, err := vs.GetCompatibleDatastores(ctx, pbmClient, resourcePool, volumeOptions.StoragePolicyID) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if volumeOptions.Datastore != "" { | ||
| if !IsUserSpecifiedDatastoreCompatible(dsRefs, volumeOptions.Datastore) { | ||
| return "", fmt.Errorf("User specified datastore: %q is not compatible with the StoragePolicy: %q requirements", volumeOptions.Datastore, volumeOptions.StoragePolicyName) | ||
|
There was a problem hiding this comment. Don't we want to say why it is not compatible? SPBM gives reasons why a particular datastore is incompatible. There was a problem hiding this comment. Yes, I have made sure it shows messages why a datastore is incompatible. There was a problem hiding this comment. Can you paste an example of what message the k8s user would see? SPBM does return CompatibilityResult.error that says what are the expected capabilities and what the storage can offer (actual capability). Do we show this to k8s user? |
||
| } | ||
| } else { | ||
| datastore = GetBestFitCompatibleDatastore(dsRefs) | ||
| } | ||
| } | ||
|
|
||
| ds, err := f.Datastore(ctx, datastore) | ||
| if err != nil { | ||
| glog.Errorf("Failed while searching for datastore %+q. err %s", datastore, err) | ||
| return "", err | ||
| } | ||
|
|
||
| if volumeOptions.VSANStorageProfileData != "" { | ||
| // Check if the datastore is VSAN if any capability requirements are specified. | ||
| // VSphere cloud provider now only supports VSAN capabilities requirements | ||
| ok, err := checkIfDatastoreTypeIsVSAN(vs.client, ds) | ||
|
|
@@ -1291,7 +1331,14 @@ func (vs *VSphere) CreateVolume(volumeOptions *VolumeOptions) (volumePath string | |
| " The policy parameters will work only with VSAN Datastore."+ | ||
| " So, please specify a valid VSAN datastore in Storage class definition.", datastore) | ||
| } | ||
| } | ||
| // Create a disk with the VSAN storage capabilities specified in the volumeOptions.VSANStorageProfileData. | ||
| // This is achieved by following steps: | ||
| // 1. Create dummy VM if not already present. | ||
| // 2. Add a new disk to the VM by performing VM reconfigure. | ||
| // 3. Detach the new disk from the dummy VM. | ||
| // 4. Delete the dummy VM. | ||
| if volumeOptions.VSANStorageProfileData != "" || volumeOptions.StoragePolicyName != "" { | ||
| // Acquire a read lock to ensure multiple PVC requests can be processed simultaneously. | ||
| cleanUpDummyVMLock.RLock() | ||
| defer cleanUpDummyVMLock.RUnlock() | ||
|
|
@@ -1562,13 +1609,11 @@ func (vs *VSphere) createDummyVM(ctx context.Context, datacenter *object.Datacen | |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // Get the folder reference for global working directory where the dummy VM needs to be created. | ||
| vmFolder, err := getFolder(ctx, vs.client, vs.cfg.Global.Datacenter, vs.cfg.Global.WorkingDir) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Failed to get the folder reference for %q with err: %+v", vs.cfg.Global.WorkingDir, err) | ||
| } | ||
| task, err := vmFolder.CreateVM(ctx, virtualMachineConfigSpec, resourcePool, nil) | ||
| if err != nil { | ||
| return nil, err | ||
|
|
@@ -1665,12 +1710,17 @@ func (vs *VSphere) createVirtualDiskWithPolicy(ctx context.Context, datacenter * | |
| FileOperation: types.VirtualDeviceConfigSpecFileOperationCreate, | ||
| } | ||
|
|
||
| storageProfileSpec := &types.VirtualMachineDefinedProfileSpec{} | ||
| // Is PBM storage policy ID is present, set the storage spec profile ID, | ||
| // else, set raw the VSAN policy string. | ||
| if volumeOptions.StoragePolicyID != "" { | ||
| storageProfileSpec.ProfileId = volumeOptions.StoragePolicyID | ||
| } else if volumeOptions.VSANStorageProfileData != "" { | ||
| storageProfileSpec.ProfileId = "" | ||
| storageProfileSpec.ProfileData = &types.VirtualMachineProfileRawData{ | ||
| ExtensionKey: "com.vmware.vim.sps", | ||
| ObjectData: volumeOptions.VSANStorageProfileData, | ||
| } | ||
| } | ||
|
|
||
| deviceConfigSpec.Profile = append(deviceConfigSpec.Profile, storageProfileSpec) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed there are some known issues with the user specifying the policy name instead of policy ID, and I think we should document this so that the VI admin/k8s admin/k8s user knows about it and how to deal with it. I'm just listing down our discussion points
*** Admin updating the policy name in vCenter could cause confusions/inconsistencies ***
This happens when VI admin changes the policy name without the knowledge of k8s users or letting the k8s users know about it. The k8s users will not be able to create volume with storage class that uses old policy name. k8s user and VI admin has to resolve such issues by finding out the latest policy name and updating the existing storage classes with latest policy name.
*** Two or more PVs could show different policy names but with the same policy ID ***
The k8s user can see the policy name and policy ID while describing PVs. If a storage policy name in vCenter is updated, then this is not reflected in the policy names shown for the existing PVs (the policy ID will be shown correctly). The k8s user has to update the storage class to use the latest policy name, and any PVs created after this point will show the correct policy name and policy ID. However, if the old PVs and new PVs are described, the old PVs will continue to show the old policy name which could cause confusions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. This would not be the right place to describe these known issues. When we announce a release with this feature that's when we would have this information written up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, how are you tracking that these known issues are not lost?