MAC address cleanup #304

alromeros · 2024-12-11T13:10:08Z

What this PR does / why we need it:

When a virtual machine is restored to an alternate namespace, the virtual machine is restored with the same MAC address as the original one. This results in a MAC address conflict if the original virtual machine is still running on the original namespace.

Though we already have a work-in-progress PR, recent interest in this feature has forced us to open a new one to address this issue. From now on, users can optionally clean a VM or VMI MAC address by setting the "velero.kubevirt.io/clear-mac-address" label in the restore object.

Cherry-picked a couple of commits from #235 to keep the original contributor's work.

/cc @esteban-ee

Which issue(s) this PR fixes:
Fixes #199

Release note:

Support clearing MAC address by label

kubevirt-bot · 2024-12-11T13:10:11Z

@alromeros: GitHub didn't allow me to request PR reviews from the following users: esteban-ee.

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

In response to this:

What this PR does / why we need it:

When a virtual machine is restored to an alternate namespace, the virtual machine is restored with the same MAC address as the original virtual machine. This results in a MAC address conflict if the original virtual machine is still running on the original namespace.

Though we already have a work-in-progress PR, recent interest in this feature has forced us to open a new one to address this issue. From now on, users can optionally clean a VM or VMI MAC address by setting the "velero.kubevirt.io/clear-mac-address" label in the restore object.

Cherry-picked a couple of commits from #235 to keep the original contributor's work.

/cc @esteban-ee

Which issue(s) this PR fixes:
Fixes #199

Release note:
Support clearing MAC address by label

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-sigs/prow repository.

alromeros · 2024-12-11T17:05:53Z

/cc @ShellyKa13 @mhenriks

mhenriks · 2024-12-13T16:49:38Z

/approve

kubevirt-bot · 2024-12-13T16:49:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

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:

~~OWNERS~~ [mhenriks]

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

ShellyKa13

Some comments are relevant to the 2 commit which are not yours.. I get the desire to use community member commits but they dont have much value.. I believe you should either delete them or squash the first 3 commits and add him as as signed by also..

ShellyKa13 · 2024-12-15T07:13:32Z

pkg/plugin/vm_restore_item_action.go

@@ -63,6 +63,10 @@ func (p *VMRestorePlugin) Execute(input *velero.RestoreItemActionExecuteInput) (
 		return nil, errors.WithStack(err)
 	}

+	for i := 0; i < len(vm.Spec.Template.Spec.Domain.Devices.Interfaces); i++ {


dont think you need this commit..

I'm not against having "incomplete" commits such as these if they showcase the development history and specifically wanted to make an exception for these as they are from another contributor. That said, we've had this conversation in the past and I'm also ok with a cleaner approach, so I'll squash them as suggested.

ShellyKa13 · 2024-12-15T07:16:58Z

pkg/plugin/vm_restore_item_action.go

@@ -63,8 +64,11 @@ func (p *VMRestorePlugin) Execute(input *velero.RestoreItemActionExecuteInput) (
 		return nil, errors.WithStack(err)
 	}

-	for i := 0; i < len(vm.Spec.Template.Spec.Domain.Devices.Interfaces); i++ {
-		vm.Spec.Template.Spec.Domain.Devices.Interfaces[i].MacAddress = ""
+	if util.IsMacAdressClearedByAnnotation(vm) {


typo Adress -> address
Also I think the name is missleading..
consider something like: ShouldClearMacAddress

Related to my comment above

ShellyKa13 · 2024-12-15T07:17:50Z

pkg/plugin/vm_restore_item_action.go

+	if util.IsMacAdressClearedByAnnotation(vm) {
+		p.log.Info("Clear virtual machine MAC addresses")
+		for i := 0; i < len(vm.Spec.Template.Spec.Domain.Devices.Interfaces); i++ {
+			vm.Spec.Template.Spec.Domain.Devices.Interfaces[i].MacAddress = ""


I think its better also to put this in a function ClearMacAddress

ShellyKa13 · 2024-12-15T07:18:48Z

pkg/util/util_test.go

+		vm       kvcore.VirtualMachine
+		expected bool
+	}{
+		{"Clear mac addres should return false",


the name of the tests should also explain what is tested i.e here: with no annotation, in the next with annotation false etc..

ShellyKa13 · 2024-12-15T07:23:35Z

pkg/util/util_test.go

-		{"Clear mac addres should return false",
-			"VirtualMachine",
-			kvcore.VirtualMachine{
+		{"Clear MAC address should return false",


you should add here "without label" and in the next test "with label"

ShellyKa13 · 2024-12-15T07:25:19Z

tests/vm_backup_test.go

+				vm.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress = originalMAC
+				return vm
+			}
+			retryOnceOnErr(updateVm(f.KvClient, f.Namespace.Name, vm.Name, update)).Should(BeNil())


maybe you should use patch here?

This is a preexisting function used in several other tests, prefer to leave it here and consider refactoring the function itself in a follow-up.

ShellyKa13 · 2024-12-15T07:26:07Z

tests/vm_backup_test.go

+		Expect(err).ToNot(HaveOccurred())
+		Expect(phase).To(Equal(velerov1api.BackupPhaseCompleted))
+
+		By("Stopping VM")


why do you need to stop the vm before you delete it?

Just a remnant from copy-pasting another test that does the same. I can remove it.

alromeros · 2024-12-16T12:42:31Z

/test pull-kvp-functional-test

awels

putting hold on it so Shelly can take a look, but if not before you want it merged, you have the lgtm
/hold

This commit allows to optionally clear the MAC address in VM and VMIs during restore. Currently, the way to go for modular backup/restore behavior in velero plugin is by using labels instead of annotations. Labels are easier to add to the restore/backup objects during creation as the velero client has a flag to include them. This commit also changes some const names to use PascalCase to match exported const naming standard. Signed-off-by: Eduardo Esteban <[email protected]> Signed-off-by: Alvaro Romero <[email protected]>

Signed-off-by: Alvaro Romero <[email protected]>

ShellyKa13 · 2024-12-18T10:02:58Z

/unhold
/lgtm
Nice work! thanks!

alromeros · 2024-12-18T10:08:06Z

Will backport this once #310 is merged, thanks @ShellyKa13!

alromeros · 2024-12-18T15:16:16Z

/cherrypick release-v0.7

alromeros · 2024-12-18T15:59:15Z

/cherrypick release-v0.7

kubevirt-bot · 2024-12-18T15:59:50Z

@alromeros: #304 failed to apply on top of branch "release-v0.7":

Applying: Clear MAC address by label
Using index info to reconstruct a base tree...
M	pkg/plugin/vm_restore_item_action.go
M	pkg/plugin/vmi_restore_item_action.go
M	pkg/util/util.go
M	pkg/util/util_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/util/util_test.go
CONFLICT (content): Merge conflict in pkg/util/util_test.go
Auto-merging pkg/util/util.go
Auto-merging pkg/plugin/vmi_restore_item_action.go
CONFLICT (content): Merge conflict in pkg/plugin/vmi_restore_item_action.go
Auto-merging pkg/plugin/vm_restore_item_action.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Clear MAC address by label

In response to this:

/cherrypick release-v0.7

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-sigs/prow repository.

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Dec 11, 2024

alromeros changed the title ~~[WIP] MAC address cleanup~~ MAC address cleanup Dec 11, 2024

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2024

kubevirt-bot requested review from mhenriks and ShellyKa13 December 11, 2024 17:05

alromeros mentioned this pull request Dec 13, 2024

Allow switching VM power state during restore #306

Merged

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 13, 2024

ShellyKa13 reviewed Dec 15, 2024

View reviewed changes

alromeros force-pushed the mac-address-cleanup branch 2 times, most recently from 3ff4b30 to 2a82153 Compare December 16, 2024 11:06

alromeros requested a review from ShellyKa13 December 16, 2024 14:41

awels approved these changes Dec 17, 2024

View reviewed changes

kubevirt-bot assigned awels Dec 17, 2024

kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Dec 17, 2024

alromeros force-pushed the mac-address-cleanup branch from 2a82153 to 7a72685 Compare December 17, 2024 15:16

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2024

Add func testing for clearing MAC address

423e3f3

Signed-off-by: Alvaro Romero <[email protected]>

alromeros force-pushed the mac-address-cleanup branch from 7a72685 to 423e3f3 Compare December 17, 2024 15:34

alromeros requested a review from awels December 17, 2024 16:02

kubevirt-bot assigned ShellyKa13 Dec 18, 2024

kubevirt-bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 18, 2024

kubevirt-bot merged commit dfff8d2 into kubevirt:main Dec 18, 2024
5 checks passed

This was referenced Dec 18, 2024

[release-0.7] MAC address cleanup #311

Merged

clear MAC addresses on restore #235

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAC address cleanup #304

MAC address cleanup #304

alromeros commented Dec 11, 2024 •

edited

Loading

kubevirt-bot commented Dec 11, 2024

alromeros commented Dec 11, 2024

mhenriks commented Dec 13, 2024

kubevirt-bot commented Dec 13, 2024

ShellyKa13 left a comment

ShellyKa13 Dec 15, 2024

alromeros Dec 16, 2024

ShellyKa13 Dec 15, 2024

alromeros Dec 16, 2024

ShellyKa13 Dec 15, 2024

ShellyKa13 Dec 15, 2024

ShellyKa13 Dec 15, 2024

ShellyKa13 Dec 15, 2024

alromeros Dec 16, 2024

ShellyKa13 Dec 15, 2024

alromeros Dec 16, 2024

alromeros commented Dec 16, 2024

awels left a comment

ShellyKa13 commented Dec 18, 2024

alromeros commented Dec 18, 2024 •

edited

Loading

alromeros commented Dec 18, 2024

alromeros commented Dec 18, 2024

kubevirt-bot commented Dec 18, 2024

MAC address cleanup #304

MAC address cleanup #304

Conversation

alromeros commented Dec 11, 2024 • edited Loading

kubevirt-bot commented Dec 11, 2024

alromeros commented Dec 11, 2024

mhenriks commented Dec 13, 2024

kubevirt-bot commented Dec 13, 2024

ShellyKa13 left a 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

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

alromeros commented Dec 16, 2024

awels left a comment

Choose a reason for hiding this comment

ShellyKa13 commented Dec 18, 2024

alromeros commented Dec 18, 2024 • edited Loading

alromeros commented Dec 18, 2024

alromeros commented Dec 18, 2024

kubevirt-bot commented Dec 18, 2024

alromeros commented Dec 11, 2024 •

edited

Loading

alromeros commented Dec 18, 2024 •

edited

Loading