VM: Add CPU model struct to domain

[crazytaxii]

What this PR does

Add latest CPU model struct to domain.

Before this PR:

type CPU struct {
	Mode     string       `xml:"mode,attr,omitempty"`
	Model    string       `xml:"model,omitempty"`
	Features []CPUFeature `xml:"feature"`
	Topology *CPUTopology `xml:"topology"`
	NUMA     *NUMA        `xml:"numa,omitempty"`
}

After this PR:

type CPUModel struct {
	Fallback string `xml:"fallback,attr,omitempty"`
	VendorID string `xml:"vendor_id,attr,omitempty"`
	Value    string `xml:",chardata"`
}

type CPU struct {
	Check    string       `xml:"check,attr,omitempty"`
	Mode     string       `xml:"mode,attr,omitempty"`
	Model    *CPUModel    `xml:"model,omitempty"`
	Features []CPUFeature `xml:"feature"`
	Topology *CPUTopology `xml:"topology"`
	NUMA     *NUMA        `xml:"numa,omitempty"`
}

According to https://libvirt.org/formatdomain.html#cpu-model-and-topology:

The optional vendor_id attribute ( Since 0.10.0 ) can be used to set the vendor id seen by the guest.

Why we need it and why it was done in this way

Update CPU model struct to latest.

Release note

NONE

[kubevirt-bot]

[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 vladikr 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:

• OWNERS

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

[kubevirt-bot]

Hi @crazytaxii. 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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[alicefr]

Is this value populated somewhere in the converter?

[crazytaxii]

Currently there is none, but libvirt domain XML has this value.

[alicefr]

Do we need this struct? Or at least all the values. You are only setting the Value in the converting and not the rest

[alicefr]

@crazytaxii why do we need this? Can you give us some details? Are you worry that the CPUModel info is parsed wrongly?

[alicefr]

/cc @victortoso seems like another change who would need libvirtxml

[kubevirt-bot]

@alicefr: GitHub didn't allow me to request PR reviews from the following users: need, would, another, change, who, seems, like.

Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @victortoso seems like another change who would need libvirtxml

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.

[vladikr]

@crazytaxii why do we need this? Can you give us some details? Are you worry that the CPUModel info is parsed wrongly?

+1
I don't see a reason to add this at this point.
We shouldn't be trying to model every field libvirt has.

[crazytaxii]

about check field:

I export a domain XML from actual running VMI:

  <cpu mode='custom' match='exact' check='full'>
    <model fallback='forbid'>Cascadelake-Server</model>
    <vendor>Intel</vendor>
  </cpu>

about CPUModel struct:

We found Windows VMs running on Hygon CPU, libvirt domain XML must be written as:

  <cpu mode='custom' match='exact' check='none'>
    <model fallback='forbid' vendor_id='AuthenticAMD'>EPYC</model>
  </cpu>

Otherwise, The Windows will blue screen. The Original CPU model field is too simple to do that.

[crazytaxii]

In our customized KubeVirt codes, I firstly detect CPU vendor of the host and define special domain XML on hosts with Hygon processors:

  <cpu mode='custom' match='exact' check='none'>
    <model fallback='forbid' vendor_id='AuthenticAMD'>EPYC</model>
  </cpu>

The Vendor ID and CPU model value is clearly customized, but the model struct is common, according to https://libvirt.org/formatdomain.html#cpu-model-and-topology. So I want to improve the scalability of Domain struct in KubeVirt.

[crazytaxii]

The problem is published on my blog, but it's written in Chinese 😥.
https://blog.crazytaxii.com/posts/libvirt_on_hygon/

[victortoso]

Hi @crazytaxii thanks for clarifying your use case.

Otherwise, The Windows will blue screen. The Original CPU model field is too simple to do that.

Just to be 100%, that means that using plain host-model with libvirt would result in blue screen, correct?

I'm inclined to think this is a libvirt bug or at least we should understand why you need to set check to none if the CPU is supported by both libvirt & QEMU.

In our customized KubeVirt codes, I firstly detect CPU vendor of the host and define special domain XML on hosts with Hygon processors:

Are you using sidecars for that? That is, even without adding extra fields to schema.go you could apply these configs with a sidecar and that should work as well.

[crazytaxii]

Just to be 100%, that means that using plain host-model with libvirt would result in blue screen, correct?

No, we use custom (the default one), Windows is still blue screen.

Are you using sidecars for that? That is, even without adding extra fields to schema.go you could apply these configs with a sidecar and that should work as well.

No, we just change the virt-launcher's convertor code.

[crazytaxii]

libvirt and qemu will pass-through the CPU vendor of host if it's undefined in domain XML. For example:

  <cpu mode='custom' match='exact' check='full'>
    <model fallback='forbid'>Cascadelake-Server</model>
    <vendor>Intel</vendor>
  </cpu>

The CPU vendor of VM will be the same as host.

[alicefr]

Are you using sidecars for that? That is, even without adding extra fields to schema.go you could apply these configs with a sidecar and that should work as well.

No, we just change the virt-launcher's convertor code.

Instead of modify virt-launcher code you should rely on sidecars as Victor suggested.

[crazytaxii]

Are you using sidecars for that? That is, even without adding extra fields to schema.go you could apply these configs with a sidecar and that should work as well.

No, we just change the virt-launcher's convertor code.

Instead of modify virt-launcher code you should rely on sidecars as Victor suggested.

OK