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.

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

Add vSphere storagecapabilities #3284

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

ido106
Copy link

@ido106 ido106 commented May 21, 2024

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

What this PR does / why we need it:
Adding storagecapabilities to vSphere provisioner.

According to the VMware documentation and the compatibility matrices, raw block volumes support only rwo access:

Use only single-access ReadWriteOnce raw block volumes. vSphere Container Storage Plug-in does not support raw block volume that use the ReadWriteMany access mode.

According to Red Hat OpenShift documentation (also from the VMware docmentation), vSphere support rwx only if the underlying vSphere environment supports the vSAN file service:

If the underlying vSphere environment supports the vSAN file service, then vSphere Container Storage Interface (CSI) Driver Operator installed by OpenShift Container Platform supports provisioning of ReadWriteMany (RWX) volumes. If vSAN file service is not configured, then ReadWriteOnce (RWO) is the only access mode available. If you do not have vSAN file service configured, and you request RWX, the volume fails to get created and an error is logged.

(maybe I can check it like here?)

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-42269

Special notes for your reviewer:

Release note:

Add storagecapabilities to vSphere provisioner

@kubevirt-bot 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 21, 2024
@kubevirt-bot kubevirt-bot requested review from akalenyu and mhenriks May 21, 2024 16:07
@kubevirt-bot
Copy link
Contributor

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-sigs/prow repository.

@ido106
Copy link
Author

ido106 commented May 21, 2024

@arnongilboa I didn't find anything about the in-tree vSphere volume except this (the string is kubernetes.io/vsphere-volume), and it says that Kubernetes will not support it in the future:

Kubernetes will deprecate the in-tree vSphere volume plug-in, and it will be removed in the future Kubernetes releases. Volumes provisioned using the vSphere in-tree plug-in will not get additional new features supported by the vSphere Container Storage Plug-in.

@ido106 ido106 changed the title Add storagecapabilities to vSphere provisioner Add vSphere storagecapabilities May 28, 2024
@arnongilboa
Copy link
Collaborator

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2024
@coveralls
Copy link

coveralls commented Jun 2, 2024

Coverage Status

coverage: 59.036% (+0.01%) from 59.023%
when pulling df31369 on ido106:vsphere_storagecapabilities
into f3d0060 on kubevirt:main.

@arnongilboa
Copy link
Collaborator

/lgtm cancel

@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2024
@@ -108,6 +108,8 @@ var CapabilitiesByProvisionerKey = map[string][]StorageCapabilities{
// Infinidat
"infinibox-csi-driver/iscsiorfibrechannel": {{rwx, block}, {rwo, block}, {rwo, file}},
"infinibox-csi-driver/nfs": {{rwx, file}, {rwo, file}},
// vSphere
"csi.vsphere.vmware.com": {{rwo, block}, {rwo, file}},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you should add {rwx, file}, although I don't see Parameters we can check rely on. Anyway, the user can always force the DV to use rwo if rwx is not supported.

Copy link
Author

Choose a reason for hiding this comment

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

Added

@ido106 ido106 force-pushed the vsphere_storagecapabilities branch from 2d7d1bb to a548d03 Compare June 2, 2024 12:56
@arnongilboa
Copy link
Collaborator

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2024
Copy link
Collaborator

@akalenyu akalenyu left a comment

Choose a reason for hiding this comment

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

So if I'm reading kubernetes-sigs/vsphere-csi-driver#2173 correctly this driver only allows RWX for a specific csi.storage.k8s.io/fstype of nfs

@arnongilboa
Copy link
Collaborator

So if I'm reading kubernetes-sigs/vsphere-csi-driver#2173 correctly this driver only allows RWX for a specific csi.storage.k8s.io/fstype of nfs

In this example sc it's nfs4. Are you sure we can trust this parameter?

@awels
Copy link
Member

awels commented Jun 3, 2024

My question is really why are we adding vsphere at all? Are we planning on supporting nested virtualization? I guess for testing it will be okay.

@akalenyu
Copy link
Collaborator

akalenyu commented Jun 3, 2024

So if I'm reading kubernetes-sigs/vsphere-csi-driver#2173 correctly this driver only allows RWX for a specific csi.storage.k8s.io/fstype of nfs

In this example sc it's nfs4. Are you sure we can trust this parameter?

Both work - https://github.com/kubernetes-sigs/vsphere-csi-driver/pull/2173/files#diff-2fcdb9d6c68925ed6f0b44f158f4b9fa8e0bba16adc38a6bf1c0844b23a66c38R211-R212

@ido106
Copy link
Author

ido106 commented Jun 3, 2024

So if I'm reading kubernetes-sigs/vsphere-csi-driver#2173 correctly this driver only allows RWX for a specific csi.storage.k8s.io/fstype of nfs

In this example sc it's nfs4. Are you sure we can trust this parameter?

According to this P.R I think the options are only nfs and nfs4:
// For ReadWriteMany or ReadOnlyMany access modes we only support nfs or nfs4 filesystem.
I also see that rwo is not supported with nfs:
expectedErrMsg := "fstype nfs4 not supported for ReadWriteOnce volume creation"

@arnongilboa
Copy link
Collaborator

So if I'm reading kubernetes-sigs/vsphere-csi-driver#2173 correctly this driver only allows RWX for a specific csi.storage.k8s.io/fstype of nfs

In this example sc it's nfs4. Are you sure we can trust this parameter?

According to this P.R I think the options are only nfs and nfs4: // For ReadWriteMany or ReadOnlyMany access modes we only support nfs or nfs4 filesystem. I also see that rwo is not supported with nfs: expectedErrMsg := "fstype nfs4 not supported for ReadWriteOnce volume creation"

The check Alex pointed supports "ext4", "nfs", "nfs4" & "".

@ido106
Copy link
Author

ido106 commented Jun 3, 2024

So if I'm reading kubernetes-sigs/vsphere-csi-driver#2173 correctly this driver only allows RWX for a specific csi.storage.k8s.io/fstype of nfs

In this example sc it's nfs4. Are you sure we can trust this parameter?

According to this P.R I think the options are only nfs and nfs4: // For ReadWriteMany or ReadOnlyMany access modes we only support nfs or nfs4 filesystem. I also see that rwo is not supported with nfs: expectedErrMsg := "fstype nfs4 not supported for ReadWriteOnce volume creation"

The check Alex pointed supports "ext4", "nfs", "nfs4" & "".

I think it's defaulting back to nfs4
https://github.com/kubernetes-sigs/vsphere-csi-driver/pull/2173/files#diff-6d525bc2369b00f94683307dfab773cfa2825c1d978730b2ee337c69e85b541dR963-R969

@akalenyu
Copy link
Collaborator

akalenyu commented Jun 3, 2024

So if I'm reading kubernetes-sigs/vsphere-csi-driver#2173 correctly this driver only allows RWX for a specific csi.storage.k8s.io/fstype of nfs

In this example sc it's nfs4. Are you sure we can trust this parameter?

According to this P.R I think the options are only nfs and nfs4: // For ReadWriteMany or ReadOnlyMany access modes we only support nfs or nfs4 filesystem. I also see that rwo is not supported with nfs: expectedErrMsg := "fstype nfs4 not supported for ReadWriteOnce volume creation"

The check Alex pointed supports "ext4", "nfs", "nfs4" & "".

Yeah I don't know why ext4 and "" are in there but sounds related to the hack they're working with.
I am thinking from our side strings.Contains(fsType, "nfs") is fair

Signed-off-by: Ido Aharon <iaharon@redhat.com>
@ido106 ido106 force-pushed the vsphere_storagecapabilities branch from a548d03 to df31369 Compare June 10, 2024 10:38
@kubevirt-bot kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. size/XS labels Jun 10, 2024
Copy link
Collaborator

@akalenyu akalenyu left a comment

Choose a reason for hiding this comment

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

/approve

@akalenyu
Copy link
Collaborator

/test all

@kubevirt-bot
Copy link
Contributor

[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:

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

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

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2024
@akalenyu
Copy link
Collaborator

/retest

2 similar comments
@akalenyu
Copy link
Collaborator

/retest

@arnongilboa
Copy link
Collaborator

/retest

@kubevirt-bot kubevirt-bot merged commit b2e51ee into kubevirt:main Jun 18, 2024
19 checks passed
@ido106
Copy link
Author

ido106 commented Jun 23, 2024

/cherry-pick release-v1.59

@kubevirt-bot
Copy link
Contributor

@ido106: only kubevirt org members may request cherry picks. If you are already part of the org, make sure to change your membership to public. Otherwise you can still do the cherry-pick manually.

In response to this:

/cherry-pick release-v1.59

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-sigs/prow repository.

@arnongilboa
Copy link
Collaborator

/cherry-pick release-v1.59

@kubevirt-bot
Copy link
Contributor

@arnongilboa: #3284 failed to apply on top of branch "release-v1.59":

Applying: Add storagecapabilities to vSphere provisioner
Using index info to reconstruct a base tree...
M	pkg/storagecapabilities/storagecapabilities.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/storagecapabilities/storagecapabilities.go
CONFLICT (content): Merge conflict in pkg/storagecapabilities/storagecapabilities.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add storagecapabilities to vSphere provisioner
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v1.59

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-sigs/prow repository.

akalenyu pushed a commit to akalenyu/containerized-data-importer that referenced this pull request Jul 11, 2024
Signed-off-by: Ido Aharon <iaharon@redhat.com>
kubevirt-bot pushed a commit that referenced this pull request Jul 12, 2024
* 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>
arnongilboa added a commit to arnongilboa/containerized-data-importer that referenced this pull request Oct 31, 2024
* Add storagecapabilities to vSphere provisioner (kubevirt#3284)

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

* Fix Dell storagecapabilities (kubevirt#3249)

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

* Add storagecapabilities to csi.huawei.com provisioner (kubevirt#3253)

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

* Add Longhorn storagecapabilities (kubevirt#3290)

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

---------

Signed-off-by: Ido Aharon <iaharon@redhat.com>
Co-authored-by: Ido Aharon <iaharon@redhat.com>
arnongilboa added a commit to arnongilboa/containerized-data-importer that referenced this pull request Oct 31, 2024
* Add storagecapabilities to vSphere provisioner (kubevirt#3284)

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

* Fix Dell storagecapabilities (kubevirt#3249)

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

* Add storagecapabilities to csi.huawei.com provisioner (kubevirt#3253)

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

* Add Longhorn storagecapabilities (kubevirt#3290)

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

---------

Signed-off-by: Ido Aharon <iaharon@redhat.com>
Co-authored-by: Ido Aharon <iaharon@redhat.com>
arnongilboa added a commit to arnongilboa/containerized-data-importer that referenced this pull request Oct 31, 2024
* Add storagecapabilities to vSphere provisioner (kubevirt#3284)

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

* Fix Dell storagecapabilities (kubevirt#3249)

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

* Add storagecapabilities to csi.huawei.com provisioner (kubevirt#3253)

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

* Add Longhorn storagecapabilities (kubevirt#3290)

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

---------

Signed-off-by: Ido Aharon <iaharon@redhat.com>
Co-authored-by: Ido Aharon <iaharon@redhat.com>
arnongilboa added a commit to arnongilboa/containerized-data-importer that referenced this pull request Oct 31, 2024
* Add storagecapabilities to vSphere provisioner (kubevirt#3284)

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

* Fix Dell storagecapabilities (kubevirt#3249)

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

* Add storagecapabilities to csi.huawei.com provisioner (kubevirt#3253)

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

* Add Longhorn storagecapabilities (kubevirt#3290)

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 pushed a commit that referenced this pull request Oct 31, 2024
* Add storagecapabilities to vSphere provisioner (#3284)



* Fix Dell storagecapabilities (#3249)



* Add storagecapabilities to csi.huawei.com provisioner (#3253)



* Add Longhorn storagecapabilities (#3290)



---------

Signed-off-by: Ido Aharon <iaharon@redhat.com>
Co-authored-by: Ido Aharon <iaharon@redhat.com>
kubevirt-bot pushed a commit that referenced this pull request Oct 31, 2024
* Add storagecapabilities to vSphere provisioner (#3284)



* Fix Dell storagecapabilities (#3249)



* Add storagecapabilities to csi.huawei.com provisioner (#3253)



* Add Longhorn storagecapabilities (#3290)



---------

Signed-off-by: Ido Aharon <iaharon@redhat.com>
Co-authored-by: Ido Aharon <iaharon@redhat.com>
kubevirt-bot pushed a commit that referenced this pull request Oct 31, 2024
* Add storagecapabilities to vSphere provisioner (#3284)



* Fix Dell storagecapabilities (#3249)



* Add storagecapabilities to csi.huawei.com provisioner (#3253)



* Add Longhorn storagecapabilities (#3290)



---------

Signed-off-by: Ido Aharon <iaharon@redhat.com>
Co-authored-by: Ido Aharon <iaharon@redhat.com>
kubevirt-bot pushed a commit that referenced this pull request Oct 31, 2024
* Add storagecapabilities to vSphere provisioner (#3284)



* Fix Dell storagecapabilities (#3249)



* Add storagecapabilities to csi.huawei.com provisioner (#3253)



* Add Longhorn storagecapabilities (#3290)



---------

Signed-off-by: Ido Aharon <iaharon@redhat.com>
Co-authored-by: Ido Aharon <iaharon@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants