search

kubevirt / kubevirt.github.io

KubeVirt website repo, documentation at https://kubevirt.io/user-guide/
https://kubevirt.io
MIT License
29 stars 109 forks source link

Add missing DataVolume accessModes in CDI lab #932

Closed stefanha closed 1 month ago

stefanha commented 4 months ago

The Fedora DataVolume example in the CDI lab does not work out of the box with KubeVirt v1.1.1 on minikube v1.32.0:

status:
  conditions:
  - lastHeartbeatTime: "2024-02-14T21:18:57Z"
    lastTransitionTime: "2024-02-14T21:18:57Z"
    message: no accessMode defined DV nor on StorageProfile for standard StorageClass
    reason: ErrClaimNotValid
    status: Unknown
    type: Bound

Add accessModes so the example works.

What this PR does / why we need it:

It makes the CDI lab from the KubeVirt website work.

Does this PR fix any issue? _(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged)_:

No

Fixes #

Special notes for your reviewer:

kubevirt-bot commented 4 months ago

Hi @stefanha. 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
awels commented 4 months ago

Maybe mention that is only needed if your storage class is not recognized by CDI.

stefanha commented 4 months ago

Maybe mention that is only needed if your storage class is not recognized by CDI.

It may have been caused by a mistake on my part (I followed the minikube KubeVirt installation instructions). Can you explain what you mean by "if your storage class is not recognized by CDI"?

awels commented 4 months ago

CDI maintains a list of known provisioners, and it automatically populates the values for those in the StorageProfiles. If you have a provisioner it does not know about, it will not automatically populate, and you have to specify it yourself.

awels commented 4 months ago

We also maintain documentation on how to set up storage profiles for provisioners that are not recognized: https://github.com/kubevirt/containerized-data-importer/blob/main/doc/storageprofile.md

akalenyu commented 4 months ago

similar issue in https://github.com/kubevirt/kubevirt/issues/11280

stefanha commented 4 months ago

similar issue in kubevirt/kubevirt#11280

Yes, looks like this is a duplicate of that issue.

It would be nice for the CDI lab to just work without further tweaks. It's one of the first experiences users may have with KubeVirt. Is there any harm in setting the access mode explicitly?

awels commented 4 months ago

There is no technical harm, and it is fully supported. But we would like for people to get in the habit of not setting the values and letting the system figure it out. If we start out with set all this explicitly then that will defeat that idea. I guess if we just note that this is just for the lab, then it is fine.

akalenyu commented 4 months ago

similar issue in kubevirt/kubevirt#11280

Yes, looks like this is a duplicate of that issue.

It would be nice for the CDI lab to just work without further tweaks. It's one of the first experiences users may have with KubeVirt. Is there any harm in setting the access mode explicitly?

Which provisioner is this? We see a surge of this type of issues lately so maybe some provisioner is becoming very common for a home lab quick test

stefanha commented 4 months ago

similar issue in kubevirt/kubevirt#11280

Yes, looks like this is a duplicate of that issue. It would be nice for the CDI lab to just work without further tweaks. It's one of the first experiences users may have with KubeVirt. Is there any harm in setting the access mode explicitly?

Which provisioner is this? We see a surge of this type of issues lately so maybe some provisioner is becoming very common for a home lab quick test

I hit this when running minikube v1.32.0 using Kubernetes v1.28.3 on Docker 24.0.7:

$ k get storageclass
NAME                 PROVISIONER                RECLAIMPOLICY   VOLUMEBINDINGMODE   ALLOWVOLUMEEXPANSION   AGE
standard (default)   k8s.io/minikube-hostpath   Delete          Immediate           false                  12d
stefanha commented 4 months ago

@awels If you prefer I can send a patch to add k8s.io/minikube-hostpath to storagecapabilities.go.

awels commented 4 months ago

I think that would be better, but maybe also just mention here that if someone is not using mini kube but another unknown storage that you need to set the access mode

stefanha commented 4 months ago

@awels I have change the commit to add a note about how to deal with the error.

akalenyu commented 4 months ago

similar issue in kubevirt/kubevirt#11280

Yes, looks like this is a duplicate of that issue. It would be nice for the CDI lab to just work without further tweaks. It's one of the first experiences users may have with KubeVirt. Is there any harm in setting the access mode explicitly?

Which provisioner is this? We see a surge of this type of issues lately so maybe some provisioner is becoming very common for a home lab quick test

I hit this when running minikube v1.32.0 using Kubernetes v1.28.3 on Docker 24.0.7:

$ k get storageclass
NAME                 PROVISIONER                RECLAIMPOLICY   VOLUMEBINDINGMODE   ALLOWVOLUMEEXPANSION   AGE
standard (default)   k8s.io/minikube-hostpath   Delete          Immediate           false                  12d

Ah ofc, minikube, thanks for submitting the patch!

kubevirt-bot commented 1 month ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

awels commented 1 month ago

/remove-lifecycle stale

awels commented 1 month ago

/lgtm

awels commented 1 month ago

@brianmcarey can you review?

brianmcarey commented 1 month ago

/cc @aburdenthehand

Can you take a look for approval?

aburdenthehand commented 1 month ago

/approve Great stuff. Thanks @stefanha

kubevirt-bot commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aburdenthehand, brianmcarey

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](https://github.com/kubevirt/kubevirt.github.io/blob/main/OWNERS)~~ [aburdenthehand] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment