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
9 changes: 0 additions & 9 deletions pkg/virt-launcher/virtwrap/live-migration-target.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (

diskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils"
cmdv1 "kubevirt.io/kubevirt/pkg/handler-launcher-com/cmd/v1"
"kubevirt.io/kubevirt/pkg/hooks"
"kubevirt.io/kubevirt/pkg/util"
"kubevirt.io/kubevirt/pkg/util/net/ip"
migrationproxy "kubevirt.io/kubevirt/pkg/virt-handler/migration-proxy"
Expand Down Expand Up @@ -171,14 +170,6 @@ func (l *LibvirtDomainManager) prepareMigrationTarget(
if err != nil {
return err
}
// TODO this should probably a OnPrepareMigration hook or something.
// 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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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'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.

if err != nil {
return fmt.Errorf("executing custom preStart hooks failed: %v", err)
}

if shouldBlockMigrationTargetPreparation(vmi) {
return fmt.Errorf("Blocking preparation of migration target in order to satisfy a functional test condition")
Expand Down