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.

Sign up for GitHub

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

Jump to bottom

design-proposal: VM video device type configuration #371

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

design-proposal: VM video device type configuration #371

wants to merge 1 commit into from

Conversation

dasionov
Copy link

@dasionov dasionov commented Dec 26, 2024 •
edited
Loading

What this PR does / why we need it:
This PR introduces a design proposal to discuss about the optimal implementation for introducing a dedicated field for a video device to be used.

We want to allow VM-owners to explicitly set the video device type when needed.

Fixes #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:

None

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Dec 26, 2024
@kubevirt-bot kubevirt-bot requested a review from dhiller December 26, 2024 16:59
@kubevirt-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jean-edouard for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@dasionov dasionov changed the title design-proposal: toggle_vga_video_device design-proposal: toggle default vga video device Dec 26, 2024
@dasionov
Copy link
Author

/cc @enp0s3

@kubevirt-bot kubevirt-bot requested a review from enp0s3 December 26, 2024 16:59
iholder101
iholder101 reviewed Dec 29, 2024
View reviewed changes
Copy link
Contributor

@iholder101 iholder101 left a comment

Choose a reason for hiding this comment

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

Thanks @dasionov!

design-proposals/toggle_vga_video_device.md Outdated Show resolved Hide resolved
design-proposals/toggle_vga_video_device.md Outdated Show resolved Hide resolved
design-proposals/toggle_vga_video_device.md Outdated
Comment on lines 33 to 39
### Option 1: Annotation-Based Toggle
**Description:** Introduce an annotation on the KubeVirt CR to specify the default video device type. The annotation kubevirt.io/default-video-device: vga can be used to default all VMs to VGA unless overridden in individual VMIs.

**Pros:**

- Minimal changes to the API schema.
- Easy to implement and maintain.
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I don't understand these pros.

Such an annotation would demand backward compatibility anyhow, and it's not much easier to implement AFAICT. To me it's equivalent to adding a non-visible field.

design-proposals/toggle_vga_video_device.md Outdated
The proposed changes have no anticipated impact on scalability capabilities of the KubeVirt framework
## Update/Rollback Compatibility
cluster admins and vm-owners that have old vms that supports only vga video device.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can VMs change video type between reboots / live-migrations? Will it break the guest?

Copy link
Author

Choose a reason for hiding this comment

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

AFAIK as long as the domain capabilities supports virtio as video type, switching between vga to virtio should not break the guest at all, (e.g in upgrade scenario)
unless of course the launcher image doesn't have the qemu-kvm-device-display-virtio-gpu (which provide support for virtio) installed, in that case the guest will break immediately at the first sync.

@dasionov dasionov force-pushed the toggle_vga_video_device_design_proposal branch from 8e566ba to 0ce94dd Compare December 29, 2024 16:16
@dasionov dasionov force-pushed the toggle_vga_video_device_design_proposal branch from 0ce94dd to 7928683 Compare December 30, 2024 14:08
@dasionov dasionov changed the title design-proposal: toggle default vga video device design-proposal: VM video device type configuration Dec 30, 2024
@dasionov dasionov force-pushed the toggle_vga_video_device_design_proposal branch 2 times, most recently from 784f719 to 0596398 Compare January 2, 2025 16:36
@dasionov
design-proposal: vm_video_device_type_field.md
f51aee9 
We want to allow VM-owners to explicitly set the video device type when
needed.

Signed-off-by: Daniel Sionov <[email protected]>
@dasionov dasionov force-pushed the toggle_vga_video_device_design_proposal branch from 0596398 to f51aee9 Compare January 2, 2025 16:37
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iholder101 iholder101 iholder101 left review comments

@dankenigsberg dankenigsberg dankenigsberg left review comments

@jean-edouard jean-edouard jean-edouard approved these changes

@aburdenthehand aburdenthehand Awaiting requested review from aburdenthehand

@dhiller dhiller Awaiting requested review from dhiller

@enp0s3 enp0s3 Awaiting requested review from enp0s3

Assignees

@jean-edouard jean-edouard

Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dasionov @kubevirt-bot @dankenigsberg @jean-edouard @iholder101