Update DataVolume doc references from the 'pvc' API to the 'storage' API #3244

EduardGomezEscandell · 2024-05-06T10:36:37Z

What this PR does / why we need it:

Updates DataVolume examples and documentation so they use the storage API instead of the pvc API, except for a couple of places where the use of pvc is relevant (to contrast it to the storage API).

Furthermore:

Removed the ReadWriteOnce argument as it is set by default when using the storage API.
Removed the storage resources requests when cloning from a PVC.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
https://issues.redhat.com/browse/CNV-38189

Special notes for your reviewer:

Release note:

NONE

kubevirt-bot · 2024-05-06T10:36:40Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

EduardGomezEscandell · 2024-05-06T10:36:53Z

/cc @alromeros

EduardGomezEscandell · 2024-05-06T14:50:25Z

/test all

EduardGomezEscandell · 2024-05-07T13:53:03Z

/retest

EduardGomezEscandell · 2024-05-08T09:19:59Z

/retest

EduardGomezEscandell · 2024-05-09T11:43:46Z

/retest

alromeros · 2024-05-10T10:51:53Z

Thanks for the PR! I think this looks good but I'd update the datavolumes.md doc a bit so we prioritize the ## Target Storage/PVC section more. We could move it above the source section and emphasize the possibility of dropping the accessMode if we use the storage api. That justifies the changes a bit more IMO.

EduardGomezEscandell · 2024-05-10T12:11:39Z

@alromeros Sounds good. I also rephrased it a bit to highlight its conveniences.

Tip

I also used this sort of banner, I think it works well to grab the attention of readers

Warning

Also this one, pretty nice too

coveralls · 2024-05-13T08:30:38Z

coverage: 59.063% (+41.8%) from 17.245%
when pulling 13c0ff5 on EduardGomezEscandell:doc-use-storage-api
into 7a49da9 on kubevirt:main.

alromeros · 2024-05-13T08:53:19Z

Looks good, thanks! Let's see what others think
/lgtm

aglitke · 2024-05-13T13:56:04Z

doc/clone-datavolume.md

-    resources:
-      requests:
-        storage: 500Mi
+  storage:


does this need a {} in order to be valid yaml?

Oops this is true, it should be

storage: {}

Good catch :)

aglitke · 2024-05-13T13:57:31Z

doc/datavolumes.md

+```
+
+### Storage
+The `storage` type is similar to `pvc` but it allows you to ommit some parameters.


s/ommit/omit/

aglitke · 2024-05-13T14:00:03Z

doc/datavolumes.md

+virtualization storage class by annotating it with
+`storageclass.kubevirt.io/is-default-virt-class` set to `"true"`.
+
+If you request storage with `volumeMode: fileSystem`, CDI will take the file system


We should word this slightly differently so say that fs overhead is applied whenever filesystem volumeMode is used in conjunction with the storage api (whether specified explicitly or chosen automatically). As you currently have it it sounds like this happens only if you include volumeMode: FileSystem in the spec.

Makes sense 👍

EduardGomezEscandell · 2024-05-15T09:15:15Z

/retest

All instances of DataVolume > Spec > PVC have been replaced with storage, except for two cases in datavolumes.md: - Data Volumes>Target Storage/PVC>PVC https://github.com/kubevirt/containerized-data-importer/blob/338bafe/doc/datavolumes.md#pvc - Data Volumes>Source>PVC source https://github.com/kubevirt/containerized-data-importer/blob/338bafe/doc/datavolumes.md#pvc-source Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

I also reworded it so that the paragraph does not start in lower-case. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

Removed from storage options, I still kept it for the pvc. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

This argument is optional. Only removed for 'storage', not 'pvc'. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

alromeros · 2024-05-27T17:37:11Z

/lgtm

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

EduardGomezEscandell · 2024-06-03T08:35:13Z

/retest-required

alromeros · 2024-06-03T14:55:45Z

/lgtm

awels

looks good one minor wording change.

doc/datavolumes.md

Co-authored-by: Alexander Wels <awels@redhat.com> Signed-off-by: Edu Gómez Escandell <edu1997xyz@gmail.com>

awels · 2024-06-06T16:01:54Z

/test pull-containerized-data-importer-e2e-ceph

awels

/approve

kubevirt-bot · 2024-06-06T16:02:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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~~ [awels]

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

EduardGomezEscandell · 2024-06-07T07:36:51Z

/retest

kgoldbla

looks fine

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels May 6, 2024

kubevirt-bot requested review from aglitke and awels May 6, 2024 10:36

kubevirt-bot added the size/L label May 6, 2024

kubevirt-bot requested a review from alromeros May 6, 2024 10:36

EduardGomezEscandell marked this pull request as ready for review May 7, 2024 13:53

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 7, 2024

kubevirt-bot requested a review from arnongilboa May 7, 2024 13:53

kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 11, 2024

kubevirt-bot assigned alromeros May 13, 2024

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

aglitke reviewed May 13, 2024

View reviewed changes

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label May 13, 2024

EduardGomezEscandell requested a review from aglitke May 13, 2024 14:27

EduardGomezEscandell added 10 commits May 17, 2024 11:30

Docs: Replace Storage with storage in doc/datavolumes.md

c4b8571

I also reworded it so that the paragraph does not start in lower-case. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

Docs: Remove optional default 'accessModes: ReadWriteOnce' from docs

451f32e

Removed from storage options, I still kept it for the pvc. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

Docs: Remove resources/requests/storage when cloning a PVC

71b2c07

This argument is optional. Only removed for 'storage', not 'pvc'. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

Replace 'pvc' with 'storage' API in manifests

9fe68fc

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

Docs: Moved 'Target Storage/PVC' section above 'Source'

e33f624

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

Docs: Rephrase the Storage target section to emphasize it over PVC

7799235

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

Fix empty field in YAML file

0657ee4

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

Rephrase prose around filesystem volume mode

4c9069d

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

Fix grammar and spelling

0e09bc2

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

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

Write acces mode in upgrade-testing-artifacts manifest

2fcf7b2

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label May 31, 2024

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

awels reviewed Jun 6, 2024

View reviewed changes

doc/datavolumes.md Outdated Show resolved Hide resolved

Update doc/datavolumes.md

13c0ff5

Co-authored-by: Alexander Wels <awels@redhat.com> Signed-off-by: Edu Gómez Escandell <edu1997xyz@gmail.com>

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

awels approved these changes Jun 6, 2024

View reviewed changes

kubevirt-bot assigned awels Jun 6, 2024

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

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

kubevirt-bot merged commit f3d0060 into kubevirt:main Jun 7, 2024
18 of 19 checks passed

EduardGomezEscandell deleted the doc-use-storage-api branch June 7, 2024 17:23

kgoldbla reviewed Jul 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update DataVolume doc references from the 'pvc' API to the 'storage' API #3244

Update DataVolume doc references from the 'pvc' API to the 'storage' API #3244

EduardGomezEscandell commented May 6, 2024

kubevirt-bot commented May 6, 2024

EduardGomezEscandell commented May 6, 2024

EduardGomezEscandell commented May 6, 2024

EduardGomezEscandell commented May 7, 2024

EduardGomezEscandell commented May 8, 2024

EduardGomezEscandell commented May 9, 2024

alromeros commented May 10, 2024

EduardGomezEscandell commented May 10, 2024 •

edited

Loading

coveralls commented May 13, 2024 •

edited

Loading

alromeros commented May 13, 2024

aglitke May 13, 2024

alromeros May 13, 2024

EduardGomezEscandell May 13, 2024

aglitke May 13, 2024

aglitke May 13, 2024

EduardGomezEscandell May 13, 2024

EduardGomezEscandell commented May 15, 2024

alromeros commented May 27, 2024

EduardGomezEscandell commented Jun 3, 2024

alromeros commented Jun 3, 2024

awels left a comment

awels commented Jun 6, 2024

awels left a comment

kubevirt-bot commented Jun 6, 2024

EduardGomezEscandell commented Jun 7, 2024

kgoldbla left a comment

Update DataVolume doc references from the 'pvc' API to the 'storage' API #3244

Update DataVolume doc references from the 'pvc' API to the 'storage' API #3244

Conversation

EduardGomezEscandell commented May 6, 2024

kubevirt-bot commented May 6, 2024

EduardGomezEscandell commented May 6, 2024

EduardGomezEscandell commented May 6, 2024

EduardGomezEscandell commented May 7, 2024

EduardGomezEscandell commented May 8, 2024

EduardGomezEscandell commented May 9, 2024

alromeros commented May 10, 2024

EduardGomezEscandell commented May 10, 2024 • edited Loading

coveralls commented May 13, 2024 • edited Loading

alromeros commented May 13, 2024

aglitke May 13, 2024

Choose a reason for hiding this comment

alromeros May 13, 2024

Choose a reason for hiding this comment

EduardGomezEscandell May 13, 2024

Choose a reason for hiding this comment

aglitke May 13, 2024

Choose a reason for hiding this comment

aglitke May 13, 2024

Choose a reason for hiding this comment

EduardGomezEscandell May 13, 2024

Choose a reason for hiding this comment

EduardGomezEscandell commented May 15, 2024

alromeros commented May 27, 2024

EduardGomezEscandell commented Jun 3, 2024

alromeros commented Jun 3, 2024

awels left a comment

Choose a reason for hiding this comment

awels commented Jun 6, 2024

awels left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Jun 6, 2024

EduardGomezEscandell commented Jun 7, 2024

kgoldbla left a comment

Choose a reason for hiding this comment

EduardGomezEscandell commented May 10, 2024 •

edited

Loading

coveralls commented May 13, 2024 •

edited

Loading