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

Update CDI lab to match the current behavior #914

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

alromeros
Copy link
Contributor

What this PR does / why we need it:

The CDI lab in kubevirt.io contains out-of-date processes and information, to the point where it recommends the creation of a PVC with import annotations instead of a DataVolume. The given example simply doesn't work with the latest CDI version.

Though we plan to eventually change the way CDI works with DataVolumes (check kubevirt/containerized-data-importer#2752 for more information), the lab should at least provide a working example so users can test CDI's latest versions.

With the adoption of populators this process will require an update again soon, but since users have been reporting issues with the current lab (kubevirt/containerized-data-importer#2830 or kubevirt/containerized-data-importer#2645 (comment)) at least we should fix it for the moment.

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels Aug 1, 2023
@alromeros alromeros changed the title [WIP] Update CDI lab to work with latest versions [WIP] Update CDI lab to match the current behavior Aug 1, 2023
@alromeros alromeros force-pushed the update-cdi-lab branch 2 times, most recently from 873e65f to dec1bbf Compare August 1, 2023 09:15
@alromeros alromeros changed the title [WIP] Update CDI lab to match the current behavior Update CDI lab to match the current behavior Aug 1, 2023
@kubevirt-bot 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 Aug 1, 2023
requests:
storage: 5Gi
storage:
accessModes:
Copy link
Member

Choose a reason for hiding this comment

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

Should we leave out the volume mode and access mode? I get that the lab is more likely to work if we choose RWO/Filesystem but is that teaching a bad behavior (since we would want RWX if possible).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can leave them out if preferred.

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2023
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2023
Copy link
Member

@cwilkers cwilkers left a comment

Choose a reason for hiding this comment

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

Thanks for this important update, I just added comments asking to bring the Fedora cloud image up to latest (I think 37 currently, with 38 in beta, but I could be outdated too)

It would also be good to update the Killercoda lab (kubevirt/katacoda-scenarios)

storage: 5Gi
source:
http:
url: "https://download.fedoraproject.org/pub/fedora/linux/releases/36/Cloud/x86_64/images/Fedora-Cloud-Base-36-1.5.x86_64.raw.xz"
Copy link
Member

Choose a reason for hiding this comment

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

Fedora 36 is getting old, since you're refreshing the lab, could you update this to 37?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Thanks!

@@ -42,13 +42,13 @@ Review the "cdi" pods that were added.

#### Use CDI to Import a Disk Image

As an example, we will import a Fedora36 Cloud Image as a PVC and launch a Virtual Machine making use of it.
First, you need to create a DataVolume that points to the source data you want to import. In this example, we'll use a DataVolume to import a Fedora36 Cloud Image into a PVC and launch a Virtual Machine making use of it.
Copy link
Member

Choose a reason for hiding this comment

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

If you update to 37, it's mentioned again here...

The CDI lab in kubevirt.io contains out-of-date processes and information, to the point where it recommends the creation of a PVC with import annotations instead of a DataVolume.

This commit updates it to match the expected usage.

Signed-off-by: Alvaro Romero <[email protected]>
@cwilkers
Copy link
Member

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2023
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cwilkers

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 Aug 29, 2023
@kubevirt-bot kubevirt-bot merged commit bab2574 into kubevirt:main Aug 29, 2023
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. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants