Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DELL storagecapabilities #3249

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Fix DELL storagecapabilities #3249

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 91 additions & 16 deletions pkg/storagecapabilities/storagecapabilities.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,28 +50,32 @@ var CapabilitiesByProvisionerKey = map[string][]StorageCapabilities{
"openshift-storage.cephfs.csi.ceph.com": {{rwx, file}},
// LINSTOR
"linstor.csi.linbit.com": createLinstorCapabilities(),
// dell-unity-csi
"csi-unity.dellemc.com": createDellUnityCapabilities(),
// PowerFlex
"csi-vxflexos.dellemc.com": createDellPowerCapabilities(),
// PowerScale
"csi-isilon.dellemc.com": createDellPowerCapabilities(),
// PowerMax
"csi-powermax.dellemc.com": createDellPowerCapabilities(),
// PowerStore
"csi-powerstore.dellemc.com": createDellPowerCapabilities(),
// DELL Unity XT
"csi-unity.dellemc.com": createDellUnityCapabilities(),
"csi-unity.dellemc.com/nfs": createAllFSCapabilities(),
// DELL PowerFlex
"csi-vxflexos.dellemc.com": createDellPowerFlexCapabilities(),
"csi-vxflexos.dellemc.com/nfs": createAllFSCapabilities(),
// DELL PowerScale
"csi-isilon.dellemc.com": createAllFSCapabilities(),
// DELL PowerMax
"csi-powermax.dellemc.com": createDellPowerMaxCapabilities(),
"csi-powermax.dellemc.com/nfs": createAllFSCapabilities(),
// DELL PowerStore
"csi-powerstore.dellemc.com": createDellPowerStoreCapabilities(),
"csi-powerstore.dellemc.com/nfs": createAllFSCapabilities(),
// storageos
"kubernetes.io/storageos": {{rwo, file}},
"storageos": {{rwo, file}},
//AWSElasticBlockStore
// AWSElasticBlockStore
"kubernetes.io/aws-ebs": {{rwo, block}},
"ebs.csi.aws.com": {{rwo, block}},
//AWSElasticFileSystem
// AWSElasticFileSystem
"efs.csi.aws.com": {{rwx, file}, {rwo, file}},
//Azure disk
// Azure disk
"kubernetes.io/azure-disk": {{rwo, block}},
"disk.csi.azure.com": {{rwo, block}},
//Azure file
// Azure file
"kubernetes.io/azure-file": {{rwx, file}},
"file.csi.azure.com": {{rwx, file}},
// GCE Persistent Disk
Expand Down Expand Up @@ -280,12 +284,55 @@ var storageClassToProvisionerKeyMapper = map[string]func(sc *storagev1.StorageCl
}
},
"csi.vsphere.vmware.com": func(sc *storagev1.StorageClass) string {
fsType := sc.Parameters["csi.storage.k8s.io/fstype"]
fsType := getFSType(sc)
Copy link
Author

@ido106 ido106 Jun 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added getFSType func for vSphere in the last push

if strings.Contains(fsType, "nfs") {
return "csi.vsphere.vmware.com/nfs"
}
return "csi.vsphere.vmware.com"
},
"csi-unity.dellemc.com": func(sc *storagev1.StorageClass) string {
// https://github.com/dell/csi-unity/blob/1f42af327f4130df65c5532f6029559e4ab579b5/samples/storageclass
switch strings.ToLower(sc.Parameters["protocol"]) {
case "nfs":
return "csi-unity.dellemc.com/nfs"
default:
return "csi-unity.dellemc.com"
}
},
"csi-powerstore.dellemc.com": func(sc *storagev1.StorageClass) string {
// https://github.com/dell/csi-powerstore/blob/76e2cb671bd3cb28aa860e9057649d1d911e1deb/samples/storageclass
fsType := getFSType(sc)
switch fsType {
case "nfs":
return "csi-powerstore.dellemc.com/nfs"
default:
return "csi-powerstore.dellemc.com"
}
},
"csi-vxflexos.dellemc.com": func(sc *storagev1.StorageClass) string {
// https://github.com/dell/csi-powerflex/tree/main/samples/storageclass
fsType := getFSType(sc)
switch fsType {
case "nfs":
return "csi-vxflexos.dellemc.com/nfs"
default:
return "csi-vxflexos.dellemc.com"
}
},
"csi-powermax.dellemc.com": func(sc *storagev1.StorageClass) string {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's ok now I'll try to add the last three to a function, because they have the same string key (csi.storage.k8s.io/fstype)

// https://github.com/dell/csi-powermax/tree/main/samples/storageclass
fsType := getFSType(sc)
switch fsType {
case "nfs":
return "csi-powermax.dellemc.com/nfs"
default:
return "csi-powermax.dellemc.com"
}
},
}

func getFSType(sc *storagev1.StorageClass) string {
return strings.ToLower(sc.Parameters["csi.storage.k8s.io/fstype"])
}

func createRbdCapabilities() []StorageCapabilities {
Expand Down Expand Up @@ -316,11 +363,39 @@ func createDellUnityCapabilities() []StorageCapabilities {
}
}

func createDellPowerCapabilities() []StorageCapabilities {
func createDellPowerMaxCapabilities() []StorageCapabilities {
return []StorageCapabilities{
{rwx, block},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akalenyu according to the Dell doc PowerMax/PoweFlwex/PowerStore suppport RWX/ROX for Filesystem volume mode with NFS only. Shouldn't we add them to the capabilities?

Copy link
Collaborator

@akalenyu akalenyu May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there's a good chance this driver needs us to differentiate over one of storage class parameters,
for example, I see FibreChannel/iSCSI/NFS examples in here:
https://github.com/dell/csi-powermax/tree/main/samples/storageclass
Which could mean that somebody using powermax-nfs will fail to create block volumes altogether

containerized-data-importer/pkg/storagecapabilities/storagecapabilities.go

Lines 292 to 294 in 8423424

func createDellPowerMaxCapabilities() []StorageCapabilities {
return []StorageCapabilities{
{rwx, block},
But I don't see anything obvious to key off in the storage class examples.

TBH usually it's best to have the storage provider make the recommendation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok now?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the way they indicate this is nfs is via csi.storage.k8s.io/fstype?
This is a little surprising since this key is normally used as a configurable to specify which filesystem you want created on top of the block device (ext4/xfs/...) For example - https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/docs/parameters.md

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After checking I think you are right. I can't find anything else that mentions nfs or protocol in the code in our context (except this but I don't think that's what we're looking for). Only DELL unity mentions the protocol explicitly as in the example here, but all the others do not.
Maybe we'll just ask a contact from DELL what the string is, if we have one?

{rwo, block},
{rwo, file},
{rox, block},
}
}

func createDellPowerFlexCapabilities() []StorageCapabilities {
return []StorageCapabilities{
{rwx, block},
{rwo, block},
{rwo, file},
{rox, block},
{rox, file},
}
}

func createAllFSCapabilities() []StorageCapabilities {
return []StorageCapabilities{
{rwx, file},
{rwo, file},
{rox, file},
}
}

func createDellPowerStoreCapabilities() []StorageCapabilities {
return []StorageCapabilities{
{rwx, block},
{rwo, block},
{rwo, file},
{rox, block},
}
}

Expand Down