Support virtio-transitional for old guests #4730

rmohr · 2020-12-30T13:42:34Z

What this PR does / why we need it:

Old guests like RHEL6 or CentOS6 require virtio-transitional in newer libvirt/qemu versions.

Add a spec.domain.devices.useVirtioTransitional boolean which indicats which version of virtio the virtual machine needs. It will then use virtio-transitional for all virtio devices of type:

RNG
Disks
Interfaces
virtio-serial controllers

The following stay with their current value:

virtio-scsi controllers have to stick with virtio-scsi
tablet input devices have to stick with virtio
Ballooning: due to https://bugzilla.redhat.com/show_bug.cgi?id=1911786

Finally if the useVirtioTransitional boolean is unset or false, we now exclititly translate virtio from the API to virtio-non-transitional to avoid ambiguities, since the term virtio is in most cases deprecated now in libvirt and can lead to different results based on different PCI(e) layouts.

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 #

Special notes for your reviewer:

A big chunk of the code changes only extract the domain converter from the domain api package into the converter package. This was necessary to properly share validation code without import cycles (and improves the code structure).

Release note:

Add spec.domain.devices.useVirtioTransitional boolean to support virtio-transitional for old guests

rmohr · 2020-12-30T13:45:39Z

/cc @dankenigsberg

rmohr · 2020-12-30T13:48:36Z

/cc @vladikr

dankenigsberg · 2020-12-30T15:09:36Z

/cc @maiqueb @AlonaKaplan

This seems like a replacement of pull #3510.
The suggested useVirtioTransitional API works for me.

rmohr · 2020-12-31T11:30:26Z

/retest

rmohr · 2020-12-31T19:31:49Z

/retest

maiqueb

The commit msg heading in bbcb652 has a small typo - s/Table/Tablet/

Other than that, you've duplicated a parameter in the RNG 'converter' function.

Everything else lgtm.

maiqueb · 2021-01-04T12:24:33Z

pkg/virt-launcher/virtwrap/api/converter.go

@@ -659,10 +663,10 @@ func Convert_v1_Watchdog_To_api_Watchdog(source *v1.Watchdog, watchdog *Watchdog
 	return fmt.Errorf("watchdog %s can't be mapped, no watchdog type specified", source.Name)
 }

-func Convert_v1_Rng_To_api_Rng(source *v1.Rng, rng *Rng, _ *ConverterContext) error {
+func Convert_v1_Rng_To_api_Rng(c *ConverterContext, _ *v1.Rng, rng *Rng, _ *ConverterContext) error {


You already have here a *ConverterContext as a nameless parameter - last parameter in the signature.

Shouldn't you just rename that one ?

I'm OK with reordering the parameters, but not w/ duplicating them.

Fixed. I missed that it was there already.

Add the boolean flag to the API. If unset or set to false, `virtio` bus will internally converted to `virtio_non_transitional` for all devices. If set to true, `virtio_transitional` will internally be selected for all `virtio` buses. Signed-off-by: Roman Mohr <rmohr@redhat.com>

maiqueb · 2021-01-04T12:43:23Z

/cc @maiqueb @AlonaKaplan

This seems like a replacement of pull #3510.

It is a replacement for the aforementioned PR.

The suggested useVirtioTransitional API works for me.

The suggested solution is in-line with what was discussed in the aforementioned PR.

The converter will set the model to `virtio-non-transitional` if `useVirtioTransitional` is undefined or false. If set to true, `virtio-transitional` models will be set. The only exception is `virtio-scsi` on the scsi controller which needs to stay this way. Additionally there exist newer devices which have no `virtio-transitional` implementation. KubeVirt does not support any of these devices at the moment. Therefore no extra protication is added for that. Signed-off-by: Roman Mohr <rmohr@redhat.com>

Signed-off-by: Roman Mohr <rmohr@redhat.com>

The converter is moved out of the package where the API is, to allow sharing validation methods between converter unit tests and functional tests. Without this change we have import cycles since importing the Domain API means importing the converter. Signed-off-by: Roman Mohr <rmohr@redhat.com>

Ensure that all components with virtio model are set to virto-transitional if requested and that the VM can still boot successfully. Signed-off-by: Roman Mohr <rmohr@redhat.com>

Signed-off-by: Roman Mohr <rmohr@redhat.com>

Tablet input devices always need to set virtio as model. Signed-off-by: Roman Mohr <rmohr@redhat.com>

virtio-transitional and virtio-non-transitional both lead to ``` {"component":"virt-launcher","level":"error","msg":"internal error: Cannot determine balloon device path","pos":"qemuMonitorInitBalloonObjectPath:1022","subcomponent":"libvirt","thread":"32","timestamp":"2020-12-31T08:40:38.917000Z"} ``` when trying to get memory stats. See https://bugzilla.redhat.com/show_bug.cgi?id=1911786 for details. Signed-off-by: Roman Mohr <rmohr@redhat.com>

rmohr · 2021-01-04T20:00:10Z

/retest

vladikr · 2021-01-05T02:37:14Z

/approve
/lgtm

kubevirt-bot · 2021-01-05T02:37:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vladikr

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

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

kubevirt-commenter-bot · 2021-01-05T09:23:03Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

rmohr · 2021-01-05T15:41:42Z

/cherrypick release-0.36

kubevirt-bot · 2021-01-05T15:42:01Z

@rmohr: #4730 failed to apply on top of branch "release-0.36":

Applying: Make the converter aware of virtio model preferences
Applying: Unit test for choosing virtio-transitional
Applying: Extract converter into its own subpackage
Using index info to reconstruct a base tree...
M	pkg/virt-launcher/virtwrap/api/converter.go
M	pkg/virt-launcher/virtwrap/api/deepcopy_generated.go
M	pkg/virt-launcher/virtwrap/manager.go
M	pkg/virt-launcher/virtwrap/manager_test.go
M	pkg/virt-launcher/virtwrap/network/BUILD.bazel
M	pkg/virt-launcher/virtwrap/network/common.go
M	pkg/virt-launcher/virtwrap/network/podinterface.go
M	tests/BUILD.bazel
M	tests/vmi_lifecycle_test.go
Falling back to patching base and 3-way merge...
Auto-merging tests/vmi_lifecycle_test.go
Auto-merging tests/BUILD.bazel
Auto-merging pkg/virt-launcher/virtwrap/network/podinterface.go
CONFLICT (content): Merge conflict in pkg/virt-launcher/virtwrap/network/podinterface.go
Auto-merging pkg/virt-launcher/virtwrap/network/common.go
Auto-merging pkg/virt-launcher/virtwrap/network/BUILD.bazel
Auto-merging pkg/virt-launcher/virtwrap/manager_test.go
Auto-merging pkg/virt-launcher/virtwrap/manager.go
Auto-merging pkg/virt-launcher/virtwrap/converter/pci-placement.go
Auto-merging pkg/virt-launcher/virtwrap/converter/converter_test.go
Auto-merging pkg/virt-launcher/virtwrap/converter/converter.go
CONFLICT (content): Merge conflict in pkg/virt-launcher/virtwrap/converter/converter.go
Auto-merging pkg/virt-launcher/virtwrap/api/deepcopy_generated.go
error: Failed to merge in the changes.
Patch failed at 0004 Extract converter into its own subpackage

In response to this:

/cherrypick release-0.36

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/test-infra repository.

andreabolognani · 2021-01-19T14:09:22Z

@rmohr sorry for being late to the party, I only found out about this PR now.

The following stay with their current value:
* `tablet` input devices have to stick with `virtio`

virtio-tablet is modern-only, so this makes sense.

I wonder what will happen if you have useVirtioTransitional set and install an old operating system such as RHEL 6, though? Since that OS doesn't have virtio 1.0 support, which is the whole reason you'd want to use transitional virtio in the first place, then your disk and network and other devices would work but your pointing device presumably wouldn't? Would it make sense to avoid using virtio-tablet in this scenario, and adding a USB tablet instead?

* Ballooning: due to https://bugzilla.redhat.com/show_bug.cgi?id=1911786

The bug has already been addressed upstream. Should we backport the fix to libvirt 6.6.0 and remove this special case? Is there enough time for that? IIUC this PR is going to be included in the upcoming 0.37, plus it has been backported to the already-released 0.36, but I have no idea what that means timeline-wise.

* `virtio-scsi` controllers have to stick with `virtio-scsi`

What's the rationale for treating virtio-scsi differently? <controller type='scsi'> supports model='virtio-(non-)transitional' same as all other devices which are not modern-only, so I would expect useVirtioTransitional to affect it too.

Finally if the useVirtioTransitional boolean is unset or false, we now exclititly translate virtio from the API to virtio-non-transitional to avoid ambiguities, since the term virtio is in most cases deprecated now in libvirt and can lead to different results based on different PCI(e) layouts.

I might have missed it, but it doesn't look like there's any check on the machine type being performed. While it's true that for q35 machines virtio and virtio-non-transitional will behave the same, for pc machines virtio will behave like virtio-transitional instead, and I'm afraid explicitly choosing virtio-non-transitional will result in broken pc machines. But perhaps that's not considered a concern for KubeVirt?

rmohr · 2021-01-19T14:22:15Z

I wonder what will happen if you have useVirtioTransitional set and install an old operating system such as RHEL 6, though? Since that OS doesn't have virtio 1.0 support, which is the whole reason you'd want to use transitional virtio in the first place, then your disk and network and other devices would work but your pointing device presumably wouldn't? Would it make sense to avoid using virtio-tablet in this scenario, and adding a USB tablet instead?

Yes, one can choose usb instead.

What's the rationale for treating virtio-scsi differently? <controller type='scsi'> supports model='virtio-(non-)transitional' same as all other devices which are not modern-only, so I would expect useVirtioTransitional to affect it too.

The documentation (https://libvirt.org/formatdomain.html#virtio-transitional-devices) said:

for SCSI controllers, virtio-scsi must be used instead of virtio for backwards compatibility reasons;

I may have misunderstood it. It probably just meant that it never had virtio as an option.

I might have missed it, but it doesn't look like there's any check on the machine type being performed. While it's true that for q35 machines virtio and virtio-non-transitional will behave the same, for pc machines virtio will behave like virtio-transitional instead, and I'm afraid explicitly choosing virtio-non-transitional will result in broken pc machines. But perhaps that's not considered a concern for KubeVirt?

I did not know that. I probably missed it in the documentation. We only support q35. However, one can whitelist other machine types, but we never test them. I guess as a workaround one would have to set useVirtionTransitional with these types. I would say that it is at least not optimal.

andreabolognani · 2021-01-19T16:05:20Z

I wonder what will happen if you have useVirtioTransitional set and install an old operating system such as RHEL 6, though? Since that OS doesn't have virtio 1.0 support, which is the whole reason you'd want to use transitional virtio in the first place, then your disk and network and other devices would work but your pointing device presumably wouldn't? Would it make sense to avoid using virtio-tablet in this scenario, and adding a USB tablet instead?

Yes, one can choose usb instead.

Does that have to happen explicitly, though? My point is that setting useVirtioTransitional should probably do that already, since by setting it the user has already made it clear that they don't want modern-only devices, and virtio-tablet is modern-only.

What's the rationale for treating virtio-scsi differently? <controller type='scsi'> supports model='virtio-(non-)transitional' same as all other devices which are not modern-only, so I would expect useVirtioTransitional to affect it too.

The documentation (https://libvirt.org/formatdomain.html#virtio-transitional-devices) said:
for SCSI controllers, virtio-scsi must be used instead of virtio for backwards compatibility reasons;
I may have misunderstood it. It probably just meant that it never had virtio as an option.

Yeah, the documentation is not very clear on the topic. I've just pushed a commit that improves upon it.

https://gitlab.com/libvirt/libvirt/-/commit/85523cfae0be52868df26463650dd8e40a156fb9

The change should be reflected to the website in a while.

I might have missed it, but it doesn't look like there's any check on the machine type being performed. While it's true that for q35 machines virtio and virtio-non-transitional will behave the same, for pc machines virtio will behave like virtio-transitional instead, and I'm afraid explicitly choosing virtio-non-transitional will result in broken pc machines. But perhaps that's not considered a concern for KubeVirt?

I did not know that. I probably missed it in the documentation. We only support q35. However, one can whitelist other machine types, but we never test them. I guess as a workaround one would have to set useVirtionTransitional with these types. I would say that it is at least not optimal.

I'm not sure what the best course of action would be for KubeVirt, but I wanted to make sure you were aware of the potential issues with this implementation.

rmohr · 2021-01-19T16:27:10Z

@andreabolognani I will prepare a follow-up PR. Thanks for all the details.

andreabolognani · 2021-01-19T16:46:59Z

@rmohr great!

What about the fix for RHBZ#1911786? Should I try to prepare a backport and update the virt-launcher container image accordingly? That'd force me to get at least somewhat familiar with the new bazel-based image building process O:-)

rmohr · 2021-01-19T17:18:51Z

What about the fix for RHBZ#1911786? Should I try to prepare a backport and update the virt-launcher container image accordingly?

A backport would be great. The more we can do to ensure the RHEL6 and CentOS6 is happy, the better.

That'd force me to get at least somewhat familiar with the new bazel-based image building process O:-)

Looking forward to your honest feedback ;)

maya-r · 2021-01-21T06:44:55Z

I am under the impression that transitional virtio supports both virtio<1.0 and virtio>=1.0
Have we considered not having a tunable for it and making it the default?

rmohr · 2021-01-21T07:22:20Z

I am under the impression that transitional virtio supports both virtio<1.0 and virtio>=1.0
Have we considered not having a tunable for it and making it the default?

virtio-transitional is the implementation of the virtio 0.9x specification. virtio-non-transitional is virtio 1.0. We basically present different hardware to the guest.

maya-r · 2021-01-21T07:30:00Z

virtio transitional is magically both 0.9 and >=1.0.
Virtio-1.0 capable drivers know to read VIRTIO_F_VERSION_1 and tell that >=1.0 is supported.

rmohr · 2021-01-21T07:42:44Z

virtio transitional is magically both 0.9 and >=1.0.
Virtio-1.0 capable drivers know to read VIRTIO_F_VERSION_1 and tell that >=1.0 is supported.

You are right, but still, from https://libvirt.org/formatdomain.html#virtio-transitional-devices:

virtio-transitional
This device can work both with virtio 0.9 and virtio 1.0 guest drivers, so it's the best choice when compatibility with older guest operating systems is desired. libvirt will plug the device into a conventional PCI slot.

virtio-non-transitional
This device can only work with virtio 1.0 guest drivers, and it's the recommended option unless compatibility with older guest operating systems is necessary. libvirt will plug the device into either a PCI Express slot or a conventional PCI slot based on the machine type, resulting in a more optimized PCI topology.

if we don't want to manage the PCI topology completely ourselves, we should choose based on the OS.

The new libvirt build contains a fix for https://bugzilla.redhat.com/show_bug.cgi?id=1918364 which is what forced virtio-balloon to be special-cased in kubevirt#4730 The new QEMU build contains assorted fixes, so let's bring that along for the ride. Signed-off-by: Andrea Bolognani <abologna@redhat.com>

enp0s3 · 2021-01-24T07:41:06Z

/cherrypick release-0.37

kubevirt-bot · 2021-01-24T07:41:27Z

@enp0s3: new pull request created: #4872

In response to this:

/cherrypick release-0.37

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/test-infra repository.

The new libvirt build contains a fix for https://bugzilla.redhat.com/show_bug.cgi?id=1918364 which is what forced virtio-balloon to be special-cased in kubevirt#4730 The new QEMU build contains assorted fixes, so let's bring that along for the ride. Signed-off-by: Andrea Bolognani <abologna@redhat.com>

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. size/XL labels Dec 30, 2020

kubevirt-bot requested review from phoracek and rnetser December 30, 2020 13:43

kubevirt-bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Dec 30, 2020

kubevirt-bot requested a review from dankenigsberg December 30, 2020 13:45

rmohr removed request for phoracek and rnetser December 30, 2020 13:48

kubevirt-bot requested a review from vladikr December 30, 2020 13:48

kubevirt-bot requested review from AlonaKaplan and maiqueb December 30, 2020 15:09

maiqueb suggested changes Jan 4, 2021

View reviewed changes

kubevirt-bot assigned maiqueb Jan 4, 2021

maiqueb mentioned this pull request Jan 4, 2021

Virtio transitional #3510

Closed

rmohr added 7 commits January 4, 2021 13:45

Unit test for choosing virtio-transitional

9e44185

Signed-off-by: Roman Mohr <rmohr@redhat.com>

Add virtio-transitional e2e test

0d3eed2

Ensure that all components with virtio model are set to virto-transitional if requested and that the VM can still boot successfully. Signed-off-by: Roman Mohr <rmohr@redhat.com>

virtio-serial controllers need the model set too

a97632c

Signed-off-by: Roman Mohr <rmohr@redhat.com>

Tablet input device only exists as virtio 1.0

419c964

Tablet input devices always need to set virtio as model. Signed-off-by: Roman Mohr <rmohr@redhat.com>

rmohr force-pushed the virtio-transitional branch from cb7386f to 9e08fd1 Compare January 4, 2021 12:48

kubevirt-bot assigned vladikr Jan 5, 2021

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2021

kubevirt-bot merged commit 1e40e5f into kubevirt:master Jan 5, 2021

enp0s3 mentioned this pull request Jan 7, 2021

[release-0.36] Support virtio-transitional for old guests #4763

Merged

rmohr mentioned this pull request Jan 20, 2021

Consider scsi controllers in virtio version decisions #4850

Merged

andreabolognani mentioned this pull request Jan 21, 2021

Support (non-)transitional virtio-balloon #4862

Merged

kubevirt-bot mentioned this pull request Jan 24, 2021

[release-0.37] Support virtio-transitional for old guests #4872

Merged

rmohr mentioned this pull request Jan 27, 2021

Failed to start centos 6.8 with kernel panic #4382

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support virtio-transitional for old guests #4730

Support virtio-transitional for old guests #4730

rmohr commented Dec 30, 2020 •

edited

rmohr commented Dec 30, 2020

rmohr commented Dec 30, 2020

dankenigsberg commented Dec 30, 2020

rmohr commented Dec 31, 2020

rmohr commented Dec 31, 2020

maiqueb left a comment

maiqueb Jan 4, 2021

rmohr Jan 4, 2021

maiqueb commented Jan 4, 2021

rmohr commented Jan 4, 2021

vladikr commented Jan 5, 2021

kubevirt-bot commented Jan 5, 2021

kubevirt-commenter-bot commented Jan 5, 2021

rmohr commented Jan 5, 2021

kubevirt-bot commented Jan 5, 2021

andreabolognani commented Jan 19, 2021

rmohr commented Jan 19, 2021 •

edited

andreabolognani commented Jan 19, 2021

rmohr commented Jan 19, 2021

andreabolognani commented Jan 19, 2021

rmohr commented Jan 19, 2021

maya-r commented Jan 21, 2021

rmohr commented Jan 21, 2021

maya-r commented Jan 21, 2021

rmohr commented Jan 21, 2021 •

edited

enp0s3 commented Jan 24, 2021

kubevirt-bot commented Jan 24, 2021

Support virtio-transitional for old guests #4730

Support virtio-transitional for old guests #4730

Conversation

rmohr commented Dec 30, 2020 • edited

rmohr commented Dec 30, 2020

rmohr commented Dec 30, 2020

dankenigsberg commented Dec 30, 2020

rmohr commented Dec 31, 2020

rmohr commented Dec 31, 2020

maiqueb left a comment

Choose a reason for hiding this comment

maiqueb Jan 4, 2021

Choose a reason for hiding this comment

rmohr Jan 4, 2021

Choose a reason for hiding this comment

maiqueb commented Jan 4, 2021

rmohr commented Jan 4, 2021

vladikr commented Jan 5, 2021

kubevirt-bot commented Jan 5, 2021

kubevirt-commenter-bot commented Jan 5, 2021

rmohr commented Jan 5, 2021

kubevirt-bot commented Jan 5, 2021

andreabolognani commented Jan 19, 2021

rmohr commented Jan 19, 2021 • edited

andreabolognani commented Jan 19, 2021

rmohr commented Jan 19, 2021

andreabolognani commented Jan 19, 2021

rmohr commented Jan 19, 2021

maya-r commented Jan 21, 2021

rmohr commented Jan 21, 2021

maya-r commented Jan 21, 2021

rmohr commented Jan 21, 2021 • edited

enp0s3 commented Jan 24, 2021

kubevirt-bot commented Jan 24, 2021

rmohr commented Dec 30, 2020 •

edited

rmohr commented Jan 19, 2021 •

edited

rmohr commented Jan 21, 2021 •

edited