refactor: virtwrap: remove/improve some code #11548

victortoso · 2024-03-19T13:45:04Z

What this PR does

Before this PR:
The logic was fine but not easy to rework it in a way a could try to fix the issue mentioned in pull #11324 so I'm trying to improve it as I go. This is a continuation from #11384, hopefully the next one is what I'm actually trying to fix :)

After this PR:

Some code that was running but without proper rationale was removed.
Some code that had a working logic was reworked to simplify it (nichotplug's 2nd and 3rd commit)

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

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

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.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

NONE

kubevirt-bot · 2024-03-19T13:45:10Z

[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 xpivarc 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

victortoso · 2024-03-19T13:46:22Z

Hi @EdDev, not urgent but because you are familiar with the code, when you have the time please take a look if I'm not mistaken or missing anything with this changes. Cheers!

victortoso · 2024-03-19T15:47:07Z

on 1.29-sig-compute

 14:09:13:   [FAILED] Default to virtio model and 10 seconds pooling
14:09:13:   Expected
14:09:13:       <api.MemBalloon>: {
14:09:13:           Model: "virtio-non-transitional",
14:09:13:           Stats: {Period: 10},
14:09:13:           Address: {Type: "pci", Domain: "0x0000", Bus: "0x04", Slot: "0x00", Function: "0x0", Controller: "", Target: "", Unit: "", UUID: "", Device: ""},
14:09:13:           Driver: nil,
14:09:13:           FreePageReporting: "on",
14:09:13:       }
14:09:13:   to equal
14:09:13:       <api.MemBalloon>: {
14:09:13:           Model: "virtio-non-transitional",
14:09:13:           Stats: {Period: 10},
14:09:13:           Address: {Type: "pci", Domain: "0x0000", Bus: "0x07", Slot: "0x00", Function: "0x0", Controller: "", Target: "", Unit: "", UUID: "", Device: ""},
14:09:13:           Driver: nil,
14:09:13:           FreePageReporting: "on",
14:09:13:       }
14:09:13:   In [It] at: tests/compute/vmidefaults.go:143 @ 03/19/24 14:09:04.826

Seems to be similar to the fix 125f4a1
I'll try to reproduce & fix.

EdDev · 2024-03-19T23:08:52Z

/cc

Also breaks the arguments per line, just to reduce the line length. Signed-off-by: Victor Toso <victortoso@redhat.com>

This simplify the logic around withNetworkIfacesResources() as on appendPlaceholderInterfacesToTheDomain(), in this two conditionals, we would return domainSpec which would make the follow up check of len() of Interfaces to be always true. Now we return early and appendPlaceholderInterfacesToTheDomain() is called only when we need to add placeholder interfaces. Signed-off-by: Victor Toso <victortoso@redhat.com>

No real need for DeepCopy as we can keep using domainSpec. Signed-off-by: Victor Toso <victortoso@redhat.com>

The api.Domain is passed by reference and set by the function. In SyncVMI() we ignore the return value and we can do the same with prepareMigrationTarget(), making better usage of the `domain` variable already existing. Signed-off-by: Victor Toso <victortoso@redhat.com>

This is just calling hooks OnDefineDomain without doing anything with the resulting Domain XML. If this was really needed, it should be using SetDomainSpecStrWithHooks() instead, which is what SyncVMI() does. Note that this happens later, when SyncVirtualMachine() from Launcher is called. Signed-off-by: Victor Toso <victortoso@redhat.com>

victortoso · 2024-03-20T09:57:38Z

From #11548 (comment)

I'll try to reproduce & fix.

The mistake I had was removing util.GetDomainSpecWithFlags(dom, libvirt.DOMAIN_XML_INACTIVE) which I deemed unnecessary if we keep using domainSpec but that ultimately failed. Thanks for these e2e tests :)

victortoso · 2024-03-20T12:21:51Z

/retest

victortoso · 2024-03-20T15:48:02Z

1.29-sig-compute it is a timeout issue this time.

12:47:23:   [FAILED] Expected success, but got an error:
12:47:23:       <expect.TimeoutError>: 
12:47:23:       expect: timer expired after 300 seconds
12:47:23:       300000000000
12:47:23:   In [It] at: tests/vmi_ignition_test.go:61 @ 03/20/24 12:46:33.654
12:47:23: ------------------------------

victortoso · 2024-03-22T09:46:41Z

/test pull-kubevirt-e2e-k8s-1.29-sig-compute

EdDev · 2024-03-25T06:47:30Z

pkg/virt-launcher/virtwrap/nichotplug.go

@@ -172,12 +172,18 @@ func indexedDomainInterfaces(domain *api.Domain) map[string]api.Interface {
 	return domainInterfaces
 }

+type SetDomainFunc func(*v1.VirtualMachineInstance, *api.DomainSpec) (cli.VirDomain, error)


Does it need to be exposed?

No, I'll change it

EdDev · 2024-03-25T06:50:17Z

pkg/virt-launcher/virtwrap/nichotplug.go

+func withNetworkIfacesResources(
+	vmi *v1.VirtualMachineInstance,
+	domainSpec *api.DomainSpec,
+	f SetDomainFunc,
+) (cli.VirDomain, error) {


The linter is configured to allow line lengths up to 140 [1]. Now that the function type is used, do we exceed it?

[1] https://github.com/kubevirt/kubevirt/blob/main/.golangci.yml#L50

Ah! We do have an agreed line length limit! That's great, I can point it out to people now! :)

To your question, I don't think we exceed after using function type but I'll double check and revert the change if it is still <= 140 chars per line. Thanks!

EdDev · 2024-03-25T07:21:57Z

pkg/virt-launcher/virtwrap/nichotplug.go

@@ -184,6 +184,11 @@ func withNetworkIfacesResources(
 	domainSpec *api.DomainSpec,
 	f SetDomainFunc,
 ) (cli.VirDomain, error) {
+	onRootComplex := vmi.Annotations[v1.PlacePCIDevicesOnRootComplex]
+	if len(vmi.Spec.Domain.Devices.Interfaces) == 0 || onRootComplex == "true" {
+		return f(vmi, domainSpec)


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?

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.

EdDev · 2024-03-25T07:28:14Z

pkg/virt-launcher/virtwrap/nichotplug.go

 	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(


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.

EdDev · 2024-03-25T07:39:29Z

pkg/virt-launcher/virtwrap/manager.go

@@ -678,7 +678,12 @@ func (l *LibvirtDomainManager) generateCloudInitEmptyISO(vmi *v1.VirtualMachineI
 //
 // The Domain.Spec can be alterned in this function and any changes
 // made to the domain will get set in libvirt after this function exits.
-func (l *LibvirtDomainManager) preStartHook(vmi *v1.VirtualMachineInstance, domain *api.Domain, generateEmptyIsos bool, options *cmdv1.VirtualMachineOptions) (*api.Domain, error) {
+func (l *LibvirtDomainManager) preStartHook(


You may be right that the method mutates the inputted domain, but it is very hard to be sure really.
From a caller perspective, before this change, I understood that it is suppose to change as it returned the object, but now a common reader would be surprised.

The problem is that it does mutate it and I would really prefer to fix that instead of making it hidden.

On the other hand, you could claim that you just express the reality here and it is better to see the problem over hiding it. I do not know what is better. :/

On the other hand, you could claim that you just express the reality here and it is better to see the problem over hiding it. I do not know what is better. :/

I'm fine either way, but we should come up with a clear way to define when input is being changed (e.g: documentation, variable prefix/suffix).

If we think about functions like append, because the slice might be nil, it'll do the allocation and return the pointer to the new slice. The same should happen when slice's cap is reached and append will allocate a new bigger chunk of memory.

I don't think that would ever happen for most functions that will do changes api.DomainSpec, that is, the initial pointer will be kept unchanged. For that reason, I think it is somewhat safe to do what I did. preStartHook will do what it advertises and any related changes will be propagated to internals of api.DomainSpec.

Again, either way is fine for me, I'm just in favour of team consistency overall ;)

EdDev · 2024-03-25T07:45:42Z

pkg/virt-launcher/virtwrap/live-migration-target.go

-	// Right now we need to call OnDefineDomain, so that additional setup, which might be done
-	// by the hook can also be done for the new target pod
-	hooksManager := hooks.GetManager()
-	_, err = hooksManager.OnDefineDomain(&domain.Spec, vmi)


You basically remove a hook point where a preparation on the pod could have been done to accept the new domain.

The hook point is not only about changing the domain-xml, it is also about doing other preparations based on the VMI and domain.
This may break backward compatibility.

But this is still to be called later on, as I mentioned in the commit log. I'm not sure we are breaking compatibility either, no migration tests have failed so far... which it doesn't mean I haven't broken something with this patch.

I'm not sure. By just reading the code, this looks out of place.

I'll revert it, just to be safe. Sadly, this was one of the changes I really wanted in because I plan to change OnDefineDomain later on and I'll need to replicate it here.... but let's do what is right.

Thanks again for the review.

kubevirt-bot · 2024-03-25T16:56:18Z

@victortoso: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-e2e-k8s-1.29-sig-compute	`ebbfa47`	link	true	`/test pull-kubevirt-e2e-k8s-1.29-sig-compute`
pull-kubevirt-e2e-k8s-1.29-sig-performance	`ebbfa47`	link	true	`/test pull-kubevirt-e2e-k8s-1.29-sig-performance`

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. I understand the commands that are listed here.

victortoso · 2024-05-03T17:44:53Z

I'll change the approach for this, so:
/close

kubevirt-bot · 2024-05-03T17:44:56Z

@victortoso: Closed this PR.

In response to this:

I'll change the approach for this, so:
/close

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.

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Mar 19, 2024

kubevirt-bot requested a review from acardace March 19, 2024 13:45

kubevirt-bot requested a review from stu-gott March 19, 2024 13:45

kubevirt-bot added the size/L label Mar 19, 2024

kubevirt-bot requested a review from EdDev March 19, 2024 23:08

victortoso added 5 commits March 20, 2024 08:14

nichotplug: use function type for the argument

565a30e

Also breaks the arguments per line, just to reduce the line length. Signed-off-by: Victor Toso <victortoso@redhat.com>

nichotplug: refactor: don't DeepCopy

574a7ff

No real need for DeepCopy as we can keep using domainSpec. Signed-off-by: Victor Toso <victortoso@redhat.com>

victortoso force-pushed the virtwrap-refactor branch from e606835 to ebbfa47 Compare March 20, 2024 09:55

kubevirt-bot added size/M and removed size/L labels Mar 20, 2024

EdDev suggested changes Mar 25, 2024

View reviewed changes

kubevirt-bot assigned EdDev Mar 25, 2024

kubevirt-bot closed this May 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: virtwrap: remove/improve some code #11548

refactor: virtwrap: remove/improve some code #11548

victortoso commented Mar 19, 2024

kubevirt-bot commented Mar 19, 2024

victortoso commented Mar 19, 2024

victortoso commented Mar 19, 2024

EdDev commented Mar 19, 2024

victortoso commented Mar 20, 2024

victortoso commented Mar 20, 2024

victortoso commented Mar 20, 2024 •

edited

victortoso commented Mar 22, 2024

EdDev Mar 25, 2024

victortoso Mar 25, 2024

EdDev Mar 25, 2024

victortoso Mar 25, 2024

EdDev Mar 25, 2024 •

edited

victortoso Mar 25, 2024

EdDev Mar 25, 2024

EdDev Mar 25, 2024

victortoso Mar 25, 2024

EdDev Mar 25, 2024

victortoso Mar 25, 2024

victortoso Mar 25, 2024

kubevirt-bot commented Mar 25, 2024

victortoso commented May 3, 2024

kubevirt-bot commented May 3, 2024

refactor: virtwrap: remove/improve some code #11548

refactor: virtwrap: remove/improve some code #11548

Conversation

victortoso commented Mar 19, 2024

What this PR does

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

Special notes for your reviewer

Checklist

Release note

kubevirt-bot commented Mar 19, 2024

victortoso commented Mar 19, 2024

victortoso commented Mar 19, 2024

EdDev commented Mar 19, 2024

victortoso commented Mar 20, 2024

victortoso commented Mar 20, 2024

victortoso commented Mar 20, 2024 • edited

victortoso commented Mar 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdDev Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubevirt-bot commented Mar 25, 2024

victortoso commented May 3, 2024

kubevirt-bot commented May 3, 2024

victortoso commented Mar 20, 2024 •

edited

EdDev Mar 25, 2024 •

edited