MAC address cleanup (#304)

* Clear MAC address by label 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]> * Add func testing for clearing MAC address Signed-off-by: Alvaro Romero <[email protected]> --------- Signed-off-by: Alvaro Romero <[email protected]> Co-authored-by: Eduartdo Esteban <[email protected]>
kubevirt · Dec 18, 2024 · dfff8d2 · dfff8d2
1 parent d6abb26
commit dfff8d2
Show file tree

Hide file tree

Showing 7 changed files with 146 additions and 0 deletions.
diff --git a/pkg/plugin/vm_restore_item_action.go b/pkg/plugin/vm_restore_item_action.go
@@ -71,6 +71,11 @@ func (p *VMRestorePlugin) Execute(input *velero.RestoreItemActionExecuteInput) (
 		vm.Spec.Running = nil
 	}
 
+	if util.ShouldClearMacAddress(input.Restore) {
+		p.log.Info("Clear virtual machine MAC addresses")
+		util.ClearMacAddress(&vm.Spec.Template.Spec)
+	}
+
 	item, err := runtime.DefaultUnstructuredConverter.ToUnstructured(vm)
 	if err != nil {
 		return nil, errors.WithStack(err)

diff --git a/pkg/plugin/vmi_restore_item_action.go b/pkg/plugin/vmi_restore_item_action.go
@@ -29,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	kvcore "kubevirt.io/api/core/v1"
 
+	"kubevirt.io/kubevirt-velero-plugin/pkg/util"
 	"kubevirt.io/kubevirt-velero-plugin/pkg/util/kvgraph"
 )
 
@@ -85,6 +86,11 @@ func (p *VMIRestorePlugin) Execute(input *velero.RestoreItemActionExecuteInput)
 		return nil, err
 	}
 
+	if util.ShouldClearMacAddress(input.Restore) {
+		p.log.Info("Clear virtual machine instance MAC addresses")
+		util.ClearMacAddress(&vmi.Spec)
+	}
+
 	// Restricted labels must be cleared otherwise the VMI will be rejected.
 	// The restricted labels contain runtime information about the underlying KVM object.
 	labels := removeRestrictedLabels(vmi.GetLabels())

diff --git a/pkg/plugin/vmi_restore_item_action_test.go b/pkg/plugin/vmi_restore_item_action_test.go
@@ -5,8 +5,10 @@ import (
 
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
+	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
 	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
@@ -50,6 +52,15 @@ func TestVmiRestoreExecute(t *testing.T) {
 						},
 					},
 				},
+				Restore: &velerov1.Restore{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-restore",
+						Namespace: "default",
+					},
+					Spec: velerov1.RestoreSpec{
+						IncludedNamespaces: []string{"default"},
+					},
+				},
 			},
 			false,
 			map[string]string{},
@@ -75,6 +86,15 @@ func TestVmiRestoreExecute(t *testing.T) {
 						},
 					},
 				},
+				Restore: &velerov1.Restore{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-restore",
+						Namespace: "default",
+					},
+					Spec: velerov1.RestoreSpec{
+						IncludedNamespaces: []string{"default"},
+					},
+				},
 			},
 			false,
 			map[string]string{

diff --git a/pkg/util/util.go b/pkg/util/util.go
@@ -32,6 +32,9 @@ const (
 	// RestoreRunStrategy indicates that the backed up VMs will be powered with the specified run strategy after restore.
 	RestoreRunStrategy = "velero.kubevirt.io/restore-run-strategy"
 
+	// ClearMacAddressLabel indicates that the MAC address should be cleared as part of the restore workflow.
+	ClearMacAddressLabel = "velero.kubevirt.io/clear-mac-address"
+
 	// VeleroExcludeLabel is used to exclude an object from Velero backups.
 	VeleroExcludeLabel = "velero.io/exclude-from-backup"
 )
@@ -328,3 +331,13 @@ func GetRestoreRunStrategy(restore *velerov1.Restore) (kvv1.VirtualMachineRunStr
 func IsMetadataBackup(backup *velerov1.Backup) bool {
 	return metav1.HasLabel(backup.ObjectMeta, MetadataBackupLabel)
 }
+
+func ShouldClearMacAddress(restore *velerov1.Restore) bool {
+	return metav1.HasLabel(restore.ObjectMeta, ClearMacAddressLabel)
+}
+
+func ClearMacAddress(vmiSpec *kvv1.VirtualMachineInstanceSpec) {
+	for i := 0; i < len(vmiSpec.Domain.Devices.Interfaces); i++ {
+		vmiSpec.Domain.Devices.Interfaces[i].MacAddress = ""
+	}
+}
diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go
@@ -8,6 +8,7 @@ import (
 	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	v1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	kvcore "kubevirt.io/api/core/v1"
 )
@@ -416,3 +417,42 @@ func TestRestorePossible(t *testing.T) {
 		})
 	}
 }
+
+func TestIsMacAddressCleared(t *testing.T) {
+	testCases := []struct {
+		name     string
+		resource string
+		restore  velerov1.Restore
+		expected bool
+	}{
+		{"Clear MAC address should return false with no label",
+			"Restore",
+			velerov1.Restore{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{},
+				},
+			},
+			false,
+		},
+		{"Clear MAC address should return true with ClearMacAddressLabel label",
+			"Restore",
+			velerov1.Restore{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						ClearMacAddressLabel: "",
+					},
+				},
+			},
+			true,
+		},
+	}
+
+	logrus.SetLevel(logrus.ErrorLevel)
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := ShouldClearMacAddress(&tc.restore)
+
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}
diff --git a/tests/framework/backup.go b/tests/framework/backup.go
@@ -223,6 +223,10 @@ func CreateRestoreForBackup(ctx context.Context, backupName, restoreName, backup
 	return CreateRestoreWithLabels(ctx, backupName, restoreName, backupNamespace, wait, nil)
 }
 
+func CreateRestoreWithClearedMACAddress(ctx context.Context, backupName, restoreName, backupNamespace string, wait bool) error {
+	return CreateRestoreWithLabels(ctx, backupName, restoreName, backupNamespace, wait, map[string]string{"velero.kubevirt.io/clear-mac-address": "true"})
+}
+
 func GetRestore(ctx context.Context, restoreName string, backupNamespace string) (*v1.Restore, error) {
 	checkCMD := exec.CommandContext(ctx, veleroCLI, "restore", "get", "-n", backupNamespace, "-o", "json", restoreName)
 

diff --git a/tests/vm_backup_test.go b/tests/vm_backup_test.go
@@ -260,6 +260,64 @@ var _ = Describe("[smoke] VM Backup", func() {
 		),
 	)
 
+	It("started VM should be restored with new MAC address", func() {
+		// creating a started VM, so it works correctly also on WFFC storage
+		var err error
+		By("Starting a VM")
+		vm, err = framework.CreateStartedVirtualMachine(f.KvClient, f.Namespace.Name, framework.CreateVmWithGuestAgent("test-vm", f.StorageClass))
+		Expect(err).ToNot(HaveOccurred())
+
+		err = framework.WaitForVirtualMachineStatus(f.KvClient, f.Namespace.Name, vm.Name, kvv1.VirtualMachineStatusRunning)
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Retrieving the original MAC address")
+		vm, err = f.KvClient.VirtualMachine(f.Namespace.Name).Get(context.TODO(), vm.Name, metav1.GetOptions{})
+		Expect(err).ToNot(HaveOccurred())
+		originalMAC := vm.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress
+		if originalMAC == "" {
+			// This means there is no KubeMacPool running. We can simply choose a random address
+			originalMAC = "DE-AD-00-00-BE-AF"
+			update := func(vm *kvv1.VirtualMachine) *kvv1.VirtualMachine {
+				vm.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress = originalMAC
+				return vm
+			}
+			retryOnceOnErr(updateVm(f.KvClient, f.Namespace.Name, vm.Name, update)).Should(BeNil())
+
+			err = framework.WaitForVirtualMachineStatus(f.KvClient, f.Namespace.Name, vm.Name, kvv1.VirtualMachineStatusRunning)
+			Expect(err).ToNot(HaveOccurred())
+		}
+
+		By("Creating backup")
+		err = framework.CreateBackupForNamespace(timeout, backupName, f.Namespace.Name, snapshotLocation, f.BackupNamespace, true)
+		Expect(err).ToNot(HaveOccurred())
+
+		phase, err := framework.GetBackupPhase(timeout, backupName, f.BackupNamespace)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(phase).To(Equal(velerov1api.BackupPhaseCompleted))
+
+		By("Deleting VM")
+		err = framework.DeleteVirtualMachine(f.KvClient, f.Namespace.Name, vm.Name)
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Creating restore")
+		err = framework.CreateRestoreWithClearedMACAddress(timeout, backupName, restoreName, f.BackupNamespace, true)
+		Expect(err).ToNot(HaveOccurred())
+
+		rPhase, err := framework.GetRestorePhase(timeout, restoreName, f.BackupNamespace)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(rPhase).To(Equal(velerov1api.RestorePhaseCompleted))
+
+		By("Verifying restored VM")
+		err = framework.WaitForVirtualMachineStatus(f.KvClient, f.Namespace.Name, vm.Name, kvv1.VirtualMachineStatusRunning)
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Retrieving the restored MAC address")
+		vm, err = f.KvClient.VirtualMachine(f.Namespace.Name).Get(context.TODO(), vm.Name, metav1.GetOptions{})
+		Expect(err).ToNot(HaveOccurred())
+		restoredMAC := vm.Spec.Template.Spec.Domain.Devices.Interfaces[0].MacAddress
+		Expect(restoredMAC).ToNot(Equal(originalMAC))
+	})
+
 	Context("VM and VMI object graph backup", func() {
 		Context("with instancetypes and preferences", func() {
 			nsDelFunc := func() {