Split vmi controller into smaller, more maintainable parts #14238

vladikr · 2025-03-14T04:04:36Z

What this PR does

Currently, the controller was split into three smaller parts: storage.go, lifecycle.go, and vmi.go.
The idea was to isolate storage-related code into storage.go, compute/lifecycle-related code into lifecycle.go, and keep the core controller logic in vmi.go

The next step is to move the storage related code into pkg/storage - but I have concerns about this change. Perhaps separating it into vmi/storage with it own OWNERS file would be a better approach.

Special notes for your reviewer

Release note

NONE

kubevirt-bot · 2025-03-14T04:04:39Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

vladikr · 2025-03-14T12:20:36Z

/cc @xpivarc

vladikr · 2025-03-17T14:09:48Z

/test all

vladikr · 2025-03-17T18:32:04Z

/test all

vladikr · 2025-03-17T21:23:26Z

/test all

fossedihelm

Looks good overall, thanks!
What I dislike is that now we have storage.go and datavolumes.go files.
And in storage.go, there are some methods about the datavolumes.
Can we merge them? or move dv methods under the right file?
Thank you!

vladikr · 2025-03-18T14:38:39Z

Looks good overall, thanks! What I dislike is that now we have storage.go and datavolumes.go files. And in storage.go, there are some methods about the datavolumes. Can we merge them? or move dv methods under the right file? Thank you!

@fossedihelm thanks. I think it's better to separate at this point. Did you mean the datavolume event handlers? (add/update/delete) or anything else?

fossedihelm · 2025-03-18T15:20:02Z

Looks good overall, thanks! What I dislike is that now we have storage.go and datavolumes.go files. And in storage.go, there are some methods about the datavolumes. Can we merge them? or move dv methods under the right file? Thank you!

@fossedihelm thanks. I think it's better to separate at this point. Did you mean the datavolume event handlers? (add/update/delete) or anything else?

Well, I was referring to the event handlers. But there is a cross-context function, listVMIsMatchingDV (the name is misleading), which is used by both pvc and dv.
I'm still in favor of merging them in the storage file, but if you prefer to keep them separate I'm fine with it :)

fossedihelm · 2025-03-19T07:53:56Z

Thank you @vladikr!
Can you drop the [WIP] from the PR title so that the do-not-merge/work-in-progress label disappears?
/lgtm

vladikr · 2025-03-20T15:00:05Z

@jean-edouard Can you please take a look?
It moves functions between files but does not make any real functional changes—this will come in the next follow-up.

vladikr · 2025-03-21T02:44:11Z

/test pull-kubevirt-e2e-k8s-1.32-sig-storage

jean-edouard

/approve

kubevirt-commenter-bot · 2025-03-21T12:40:53Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.30-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.32-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.30-sig-network
/test pull-kubevirt-e2e-k8s-1.30-sig-storage
/test pull-kubevirt-e2e-k8s-1.30-sig-compute
/test pull-kubevirt-e2e-k8s-1.30-sig-operator
/test pull-kubevirt-e2e-k8s-1.31-sig-network
/test pull-kubevirt-e2e-k8s-1.31-sig-storage
/test pull-kubevirt-e2e-k8s-1.31-sig-compute
/test pull-kubevirt-e2e-k8s-1.31-sig-operator

fossedihelm · 2025-03-21T17:02:24Z

Quite strange how the storage is failing even if the changes just involve function moving and minor refactorings.

jean-edouard · 2025-03-21T19:44:54Z

/hold

vladikr · 2025-03-21T19:58:23Z

yup, something is not right with this test...

vladikr · 2025-03-21T21:31:10Z

@jean-edouard the test itself is passing just fine. Looks like the cleanup is causing the issue.

kubevirt-bot · 2025-03-22T16:33:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jean-edouard

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:

~~pkg/virt-controller/watch/vmi/OWNERS~~ [jean-edouard]
~~tests/storage/OWNERS~~ [jean-edouard]

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

iholder101

Thanks a lot @vladikr! Looks awesome overall.

iholder101 · 2025-03-23T16:30:53Z

pkg/virt-controller/watch/vmi/storage.go

@@ -375,3 +375,38 @@ func (c *Controller) getFilesystemOverhead(pvc *k8sv1.PersistentVolumeClaim) (vi
 	}
 	return storagetypes.GetFilesystemOverhead(pvc.Spec.VolumeMode, pvc.Spec.StorageClassName, cdiConfig)
 }
+
+func (c *Controller) syncVolumesUpdate(vmi *virtv1.VirtualMachineInstance) {


tiny nit: this belongs to the previous commit.

iholder101 · 2025-03-23T16:40:47Z

tests/storage/migration.go

@@ -747,7 +747,7 @@ var _ = Describe(SIG("Volumes update with migration", decorators.RequiresTwoSche
 					return controller.NewVirtualMachineConditionManager().HasCondition(
 						vm, virtv1.VirtualMachineManualRecoveryRequired)
 				}).WithTimeout(120 * time.Second).WithPolling(time.Second).Should(BeFalse())
-				libwait.WaitForVMIPhase(vmi, []v1.VirtualMachineInstancePhase{v1.Running})
+				Eventually(matcher.ThisVMI(vmi), 360*time.Second, time.Second).Should(matcher.BeInPhase(v1.Running))


I fail to understand how this is related.
Should these failures concern us that some behavior was changed in this PR?

@iholder101 no, the WaitForVMIPhase looks at weird warnings and fails on them. For example `Unexpected Warning event received: testvmi-klbkg,f35c019b-b555-4665-a08f-ea219efa6a63: Failed to delete virtual machine pod virt-launcher-testvmi-klbkg-qn8zd
Obviously, all pods are deleted in this test, and it is complete. I think the warning is related to garbage collection. I was watching the pods manually, there were no delays or failures.

This is most certainly not related to your changes, but still concerning.
I've opened an issue to track this: #14326.

This move storage related functions and event handlers to storage.go Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

This moves all lifecycle related functions and event handlers into lifecycle.go Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

fossedihelm · 2025-03-24T09:23:18Z

/lgtm
Thanks!
I leave it to @iholder101 to unhold if he thinks it's ok

kubevirt-commenter-bot · 2025-03-24T09:23:24Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.30-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.32-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.30-sig-network
/test pull-kubevirt-e2e-k8s-1.30-sig-storage
/test pull-kubevirt-e2e-k8s-1.30-sig-compute
/test pull-kubevirt-e2e-k8s-1.30-sig-operator
/test pull-kubevirt-e2e-k8s-1.31-sig-network
/test pull-kubevirt-e2e-k8s-1.31-sig-storage
/test pull-kubevirt-e2e-k8s-1.31-sig-compute
/test pull-kubevirt-e2e-k8s-1.31-sig-operator

iholder101 · 2025-03-24T10:45:05Z

Thank you very much @vladikr and everyone else involved!
/hold cancel

iholder101 · 2025-03-24T10:46:01Z

FYI @ShellyKa13 @akalenyu

vladikr · 2025-03-24T21:19:39Z

/retest

kubevirt-commenter-bot · 2025-03-25T06:52:47Z

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

kubevirt-bot added do-not-merge/work-in-progress release-note-none dco-signoff: yes labels Mar 14, 2025

kubevirt-bot requested review from fossedihelm and victortoso March 14, 2025 04:04

kubevirt-bot added size/XXL area/controller sig/compute labels Mar 14, 2025

kubevirt-bot requested a review from xpivarc March 14, 2025 12:20

vladikr force-pushed the split_vmi_controller branch from 5b2571d to 8119bee Compare March 17, 2025 18:01

vladikr force-pushed the split_vmi_controller branch from 8119bee to 9710467 Compare March 17, 2025 21:22

vladikr marked this pull request as ready for review March 17, 2025 21:23

kubevirt-bot requested review from Barakmor1 and lyarwood March 17, 2025 21:24

dosubot bot added the sig/storage label Mar 17, 2025

vladikr force-pushed the split_vmi_controller branch 2 times, most recently from 646d244 to 07e1084 Compare March 17, 2025 22:32

fossedihelm reviewed Mar 18, 2025

View reviewed changes

vladikr force-pushed the split_vmi_controller branch from 07e1084 to da01c3c Compare March 18, 2025 19:38

kubevirt-bot assigned fossedihelm Mar 19, 2025

kubevirt-bot added the lgtm label Mar 19, 2025

vladikr changed the title ~~[WIP] Split vmi controller into smaller, more maintainable parts~~ Split vmi controller into smaller, more maintainable parts Mar 19, 2025

jean-edouard reviewed Mar 21, 2025

View reviewed changes

kubevirt-bot added the approved label Mar 21, 2025

kubevirt-bot added the do-not-merge/hold label Mar 21, 2025

vladikr force-pushed the split_vmi_controller branch from da01c3c to 02c12a2 Compare March 22, 2025 16:33

kubevirt-bot removed the lgtm label Mar 22, 2025

iholder101 reviewed Mar 23, 2025

View reviewed changes

vladikr added 5 commits March 23, 2025 13:18

move hasPendingPods and requeueAfter where they belong

536a209

Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

virt-controller-vmi: separate lifecycle related functions

7aaf668

This moves all lifecycle related functions and event handlers into lifecycle.go Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

virt-controller-vmi: move datavolume event handlers to datavolume

0d1c6b9

Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

adjust storage migration functional test to wait for vmi phase

Loading
Loading status checks…

db53a5d

Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>

vladikr force-pushed the split_vmi_controller branch from 02c12a2 to db53a5d Compare March 23, 2025 17:18

kubevirt-bot added the lgtm label Mar 24, 2025

kubevirt-bot removed the do-not-merge/hold label Mar 24, 2025

kubevirt-bot merged commit 4d52739 into kubevirt:main Mar 25, 2025
40 checks passed

ianb-mp mentioned this pull request May 7, 2025

VMI: Sort network interfaces to avoid patch flood #14613

Merged

8 tasks

Split vmi controller into smaller, more maintainable parts #14238

Split vmi controller into smaller, more maintainable parts #14238

Conversation

vladikr commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Special notes for your reviewer

Release note

Uh oh!

kubevirt-bot commented Mar 14, 2025

Uh oh!

vladikr commented Mar 14, 2025

Uh oh!

vladikr commented Mar 17, 2025

Uh oh!

vladikr commented Mar 17, 2025

Uh oh!

vladikr commented Mar 17, 2025

Uh oh!

fossedihelm left a comment

Choose a reason for hiding this comment

Uh oh!

vladikr commented Mar 18, 2025

Uh oh!

fossedihelm commented Mar 18, 2025

Uh oh!

fossedihelm commented Mar 19, 2025

Uh oh!

vladikr commented Mar 20, 2025

Uh oh!

vladikr commented Mar 21, 2025

Uh oh!

jean-edouard left a comment

Choose a reason for hiding this comment

Uh oh!

kubevirt-commenter-bot commented Mar 21, 2025

Uh oh!

fossedihelm commented Mar 21, 2025

Uh oh!

jean-edouard commented Mar 21, 2025

Uh oh!

vladikr commented Mar 21, 2025

Uh oh!

vladikr commented Mar 21, 2025

Uh oh!

kubevirt-bot commented Mar 22, 2025

Uh oh!

iholder101 left a comment

Choose a reason for hiding this comment

Uh oh!

iholder101 Mar 23, 2025

Choose a reason for hiding this comment

Uh oh!

vladikr Mar 23, 2025

Choose a reason for hiding this comment

Uh oh!

iholder101 Mar 23, 2025

Choose a reason for hiding this comment

Uh oh!

vladikr Mar 23, 2025

Choose a reason for hiding this comment

Uh oh!

iholder101 Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fossedihelm commented Mar 24, 2025

Uh oh!

kubevirt-commenter-bot commented Mar 24, 2025

Uh oh!

iholder101 commented Mar 24, 2025

Uh oh!

iholder101 commented Mar 24, 2025

Uh oh!

vladikr commented Mar 24, 2025

Uh oh!

kubevirt-commenter-bot commented Mar 25, 2025

Uh oh!

Uh oh!

vladikr commented Mar 14, 2025 •

edited

Loading