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

refactor: virtwrap: remove/improve some code #11548

Closed
wants to merge 5 commits into from
Closed

refactor: virtwrap: remove/improve some code #11548

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
565a30e
nichotplug: use function type for the argument
victortoso Mar 19, 2024
b40f027
nichotplug: early return on special cases
victortoso Mar 19, 2024
574a7ff
nichotplug: refactor: don't DeepCopy
victortoso Mar 19, 2024
55cb71c
virtwrap: drop return of api.Domain in preStartHook()
victortoso Mar 18, 2024
ebbfa47
virtwrap: remove unneeded call to OnDefineDomain
victortoso Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 13 additions & 11 deletions pkg/virt-launcher/virtwrap/nichotplug.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ import (
"fmt"
"strings"

"kubevirt.io/kubevirt/pkg/network/namescheme"

"libvirt.org/go/libvirt"

"kubevirt.io/kubevirt/pkg/network/namescheme"

v1 "kubevirt.io/api/core/v1"
"kubevirt.io/client-go/log"

Expand Down Expand Up @@ -189,38 +189,40 @@ func withNetworkIfacesResources(
return f(vmi, domainSpec)
Copy link
Member

@EdDev EdDev Mar 25, 2024 •
edited

Choose a reason for hiding this comment

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

I'm contemplating about this one for a while now.

First, I want to admit that I seem to have came up with this terrible function. It does not look good at all, hiding the domain object creation.

The change here results in a logic change, where we will not always call the hook twice.
On one hand this makes sense, but at the same time I am a bit concern someone (sidecar logic authors) may assume we call it only once and depend on it.
In the past it was called many times, on each change to the libvirt metadata (which carried information we wanted to preserve), but now the only place we still call it more than once is here.

I feel we need to do something to compensate to balance this now. Perhaps adding text on the hook package and relevant methods to warn about this. Even making sure it appears on the documentation/examples.

Lastly, I think we should add this "filter" outside and never call withNetworkIfacesResources if this is the case. The whole point of this function was to process the logic of calling things twice, but now it has a condition that it does not and makes it less focused.

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am a bit concern someone (sidecar logic authors) may assume we call it only once and depend on it.

We will document kubevirt/community#252 about calling the sidecar multiple times.

Lastly, I think we should add this "filter" outside and never call withNetworkIfacesResources if this is the case. The whole point of this function was to process the logic of calling things twice, but now it has a condition that it does not and makes it less focused.

WDYT?

I like it. I'll change it.

}

domainSpecWithIfacesResource := appendPlaceholderInterfacesToTheDomain(vmi, domainSpec)
dom, err := f(vmi, domainSpecWithIfacesResource)
originalIfaces := appendPlaceholderInterfacesToTheDomain(vmi, domainSpec)
dom, err := f(vmi, domainSpec)
if err != nil {
return nil, err
}

if len(domainSpec.Devices.Interfaces) == len(domainSpecWithIfacesResource.Devices.Interfaces) {
if len(domainSpec.Devices.Interfaces) == len(originalIfaces) {
return dom, nil
}

domainSpecWithoutIfacePlaceholders, err := util.GetDomainSpecWithFlags(dom, libvirt.DOMAIN_XML_INACTIVE)
if err != nil {
return nil, err
}
domainSpecWithoutIfacePlaceholders.Devices.Interfaces = domainSpec.Devices.Interfaces
domainSpecWithoutIfacePlaceholders.Devices.Interfaces = originalIfaces
// Only the devices are taken into account because some parameters are not assured to be returned when
// getting the domain spec (e.g. the `qemu:commandline` section).
domainSpecWithoutIfacePlaceholders.Devices.DeepCopyInto(&domainSpec.Devices)

return f(vmi, domainSpec)
}

func appendPlaceholderInterfacesToTheDomain(vmi *v1.VirtualMachineInstance, domainSpec *api.DomainSpec) *api.DomainSpec {
domainSpecWithIfacesResource := domainSpec.DeepCopy()
func appendPlaceholderInterfacesToTheDomain(vmi *v1.VirtualMachineInstance, domainSpec *api.DomainSpec) []api.Interface {
originalIfaces := make([]api.Interface, len(domainSpec.Devices.Interfaces))
copy(originalIfaces, domainSpec.Devices.Interfaces)

interfacePlaceholderCount := ReservedInterfaces - len(vmi.Spec.Domain.Devices.Interfaces)
for i := 0; i < interfacePlaceholderCount; i++ {
domainSpecWithIfacesResource.Devices.Interfaces = append(
domainSpecWithIfacesResource.Devices.Interfaces,
domainSpec.Devices.Interfaces = append(
Copy link
Member

Choose a reason for hiding this comment

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

This change causes the function to be mutable, having the side-effect of changing the inputted domain spec. I think we should avoid doing things like this as much as possible.

I prefer the input not to mutate and only the return value to have new values.
This is what I would expect from other objects/functions to behave like.

domainSpec.Devices.Interfaces,
newInterfacePlaceholder(i, converter.InterpretTransitionalModelType(vmi.Spec.Domain.Devices.UseVirtioTransitional)),
)
}
return domainSpecWithIfacesResource
return originalIfaces
}

func newInterfacePlaceholder(index int, modelType string) api.Interface {
Expand Down