Fix DELL storagecapabilities #3249

ido106 · 2024-05-08T13:19:39Z

Signed-off-by: Ido Aharon iaharon@redhat.com

What this PR does / why we need it:
The current storagecapabilities for Dell PowerMax, PowerFlex, PowerScale and PowerStore are defined as:
{ {rwx, block}, {rwo, block}, {rwo,file} }

According to the DELL documentation, it should be:

PowerMax:
{ {rwx, block}, {rwo, block}, {rwo, file}, {rox, block} }
PowerFlex:
{ {rwx, block}, {rwo, block}, {rwo, file}, {rox, block}, {rox, file} }
PowerScale (block volume mode is not supported):
{ {rwx, file}, {rwo, file}, {rox, file} }
PowerStore:
{ {rwx, block}, {rwo, block}, {rwo, file}, {rox, block} }

If the nfs protocol is used, it is { {rwx, block}, {rwo, block}, {rwo, file} } for all types (except for PowerScale)

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #CNV-41538

Special notes for your reviewer:

Release note:

Fix DELL storagecapabilities

kubevirt-bot · 2024-05-08T13:19:47Z

Hi @ido106. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

arnongilboa

@ido106 , please keep the capabilities sorted as the existing ones:

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

Update also the PR & commit messages accordingly.

arnongilboa · 2024-05-09T07:54:03Z

pkg/storagecapabilities/storagecapabilities.go

@@ -289,11 +289,39 @@ func createDellUnityCapabilities() []StorageCapabilities {
 	}
 }

-func createDellPowerCapabilities() []StorageCapabilities {
+func createDellPowerMaxCapabilities() []StorageCapabilities {
 	return []StorageCapabilities{
 		{rwx, block},


@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?

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

Is it ok now?

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

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?

ido106 · 2024-05-21T13:47:40Z

pkg/storagecapabilities/storagecapabilities.go

+			return "csi-vxflexos.dellemc.com"
+		}
+	},
+	"csi-powermax.dellemc.com": func(sc *storagev1.StorageClass) string {


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)

akalenyu

/approve

kubevirt-bot · 2024-06-10T10:49:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akalenyu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [akalenyu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

akalenyu · 2024-06-10T10:51:44Z

/test all

coveralls · 2024-06-10T11:27:50Z

coverage: 59.041% (+0.6%) from 58.475%
when pulling 3ded90c on ido106:dell_storagecapabailities
into dd07461 on kubevirt:main.

ido106 · 2024-06-10T12:01:44Z

At last we used the csi.storage.k8s.io/fstype parameter because this is how DELL differentiate between the types:

Hi @iaharon,
That parameter is not uniform among the drivers. IMHO to detect NFS we should rely on csi.storage.k8s.io/fstype which is more of a canonical name : https://kubernetes-csi.github.io/docs/external-provisioner.html#storageclass-parameters

ido106 · 2024-06-10T12:03:38Z

pkg/storagecapabilities/storagecapabilities.go

+	},
+	"csi-powerstore.dellemc.com": func(sc *storagev1.StorageClass) string {
+		// https://github.com/dell/csi-powerstore/blob/76e2cb671bd3cb28aa860e9057649d1d911e1deb/samples/storageclass
+		switch strings.ToLower(sc.Parameters["csi.storage.k8s.io/fstype"]) {


I'm passing it to a function because this line repeats itself

According to Dell doc: https://dell.github.io/csm-docs/docs/csidriver/ Signed-off-by: Ido Aharon <iaharon@redhat.com>

ido106 · 2024-06-23T08:44:43Z

pkg/storagecapabilities/storagecapabilities.go

@@ -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)


Just added getFSType func for vSphere in the last push

awels · 2024-06-26T12:51:39Z

/lgtm
/hold
If @akalenyu is okay with this he can take hold off.

coveralls · 2024-06-26T12:59:54Z

coverage: 59.048% (+0.01%) from 59.036%
when pulling 027ba46 on ido106:dell_storagecapabailities
into 301f0ab on kubevirt:main.

akalenyu · 2024-06-26T13:12:42Z

/hold cancel

According to Dell doc: https://dell.github.io/csm-docs/docs/csidriver/ Signed-off-by: Ido Aharon <iaharon@redhat.com>

* Add storagecapabilities to vSphere provisioner (#3284) Signed-off-by: Ido Aharon <iaharon@redhat.com> * Fix Dell storagecapabilities (#3249) According to Dell doc: https://dell.github.io/csm-docs/docs/csidriver/ Signed-off-by: Ido Aharon <iaharon@redhat.com> * Add storagecapabilities to csi.huawei.com provisioner (#3253) Signed-off-by: Ido Aharon <iaharon@redhat.com> --------- Signed-off-by: Ido Aharon <iaharon@redhat.com> Co-authored-by: Ido Aharon <iaharon@redhat.com>

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels May 8, 2024

kubevirt-bot added the size/M label May 8, 2024

kubevirt-bot requested review from akalenyu and arnongilboa May 8, 2024 13:20

ido106 changed the title ~~Change Dell storagecapabilities~~ Change DELL storagecapabilities May 8, 2024

arnongilboa reviewed May 8, 2024

View reviewed changes

ido106 force-pushed the dell_storagecapabailities branch 2 times, most recently from da384e6 to 3f9d898 Compare May 8, 2024 14:10

kubevirt-bot added size/XXL and removed size/M labels May 8, 2024

ido106 force-pushed the dell_storagecapabailities branch from 3f9d898 to 8423424 Compare May 8, 2024 14:17

kubevirt-bot added size/M and removed size/XXL labels May 8, 2024

ido106 changed the title ~~Change DELL storagecapabilities~~ Fix DELL storagecapabilities May 8, 2024

arnongilboa reviewed May 9, 2024

View reviewed changes

ido106 force-pushed the dell_storagecapabailities branch from 8423424 to e4aa822 Compare May 21, 2024 13:35

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2024

ido106 force-pushed the dell_storagecapabailities branch from e4aa822 to 3ded90c Compare May 21, 2024 13:42

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2024

ido106 commented May 21, 2024

View reviewed changes

akalenyu reviewed Jun 10, 2024

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2024

ido106 commented Jun 10, 2024

View reviewed changes

ido106 force-pushed the dell_storagecapabailities branch from 3ded90c to 9e2e9d1 Compare June 10, 2024 15:48

kubevirt-bot added size/L and removed size/M labels Jun 10, 2024

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2024

ido106 force-pushed the dell_storagecapabailities branch from 9e2e9d1 to 08fb4cf Compare June 20, 2024 13:20

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2024

Fix Dell storagecapabilities

027ba46

According to Dell doc: https://dell.github.io/csm-docs/docs/csidriver/ Signed-off-by: Ido Aharon <iaharon@redhat.com>

ido106 force-pushed the dell_storagecapabailities branch from 08fb4cf to 027ba46 Compare June 23, 2024 08:41

ido106 commented Jun 23, 2024

View reviewed changes

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2024

kubevirt-bot assigned awels Jun 26, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2024

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2024

kubevirt-bot merged commit 64d0d16 into kubevirt:main Jun 26, 2024
17 of 18 checks passed

ido106 mentioned this pull request Jun 27, 2024

Add Huawei storagecapabilities #3253

Merged

akalenyu pushed a commit to akalenyu/containerized-data-importer that referenced this pull request Jul 11, 2024

Fix Dell storagecapabilities (kubevirt#3249)

747399f

According to Dell doc: https://dell.github.io/csm-docs/docs/csidriver/ Signed-off-by: Ido Aharon <iaharon@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DELL storagecapabilities #3249

Fix DELL storagecapabilities #3249

ido106 commented May 8, 2024 •

edited

Loading

kubevirt-bot commented May 8, 2024

arnongilboa left a comment •

edited

Loading

arnongilboa May 9, 2024

akalenyu May 20, 2024 •

edited

Loading

ido106 May 21, 2024

akalenyu May 22, 2024

ido106 May 26, 2024

ido106 May 21, 2024

akalenyu left a comment

kubevirt-bot commented Jun 10, 2024

akalenyu commented Jun 10, 2024

coveralls commented Jun 10, 2024

ido106 commented Jun 10, 2024

ido106 Jun 10, 2024

ido106 Jun 23, 2024 •

edited

Loading

awels commented Jun 26, 2024

coveralls commented Jun 26, 2024

akalenyu commented Jun 26, 2024

	func createDellPowerMaxCapabilities() []StorageCapabilities {
	return []StorageCapabilities{
	{rwx, block},

Fix DELL storagecapabilities #3249

Fix DELL storagecapabilities #3249

Conversation

ido106 commented May 8, 2024 • edited Loading

kubevirt-bot commented May 8, 2024

arnongilboa left a comment • edited Loading

Choose a reason for hiding this comment

arnongilboa May 9, 2024

Choose a reason for hiding this comment

akalenyu May 20, 2024 • edited Loading

Choose a reason for hiding this comment

ido106 May 21, 2024

Choose a reason for hiding this comment

akalenyu May 22, 2024

Choose a reason for hiding this comment

ido106 May 26, 2024

Choose a reason for hiding this comment

ido106 May 21, 2024

Choose a reason for hiding this comment

akalenyu left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Jun 10, 2024

akalenyu commented Jun 10, 2024

coveralls commented Jun 10, 2024

ido106 commented Jun 10, 2024

ido106 Jun 10, 2024

Choose a reason for hiding this comment

ido106 Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

awels commented Jun 26, 2024

coveralls commented Jun 26, 2024

akalenyu commented Jun 26, 2024

ido106 commented May 8, 2024 •

edited

Loading

arnongilboa left a comment •

edited

Loading

akalenyu May 20, 2024 •

edited

Loading

ido106 Jun 23, 2024 •

edited

Loading