From ee4d2b62041bd45cb8444f61464c9839db6b2dae Mon Sep 17 00:00:00 2001 From: Bryan Venteicher Date: Fri, 6 Dec 2024 11:21:19 -0600 Subject: [PATCH] Remove disk related backup field from ExtraConfig when empty Previously we'd base64+gzip "null" for this case but it is clearer to remove the EC entry in that case. --- .../vsphere/virtualmachine/backup.go | 66 ++++++++----------- .../vsphere/virtualmachine/backup_test.go | 61 +++++++++++++++++ 2 files changed, 88 insertions(+), 39 deletions(-) diff --git a/pkg/providers/vsphere/virtualmachine/backup.go b/pkg/providers/vsphere/virtualmachine/backup.go index 785743bd2..d507e3821 100644 --- a/pkg/providers/vsphere/virtualmachine/backup.go +++ b/pkg/providers/vsphere/virtualmachine/backup.go @@ -117,13 +117,14 @@ func BackupVirtualMachine(opts BackupVirtualMachineOptions) (result error) { }) } - pvcDiskData, classicDiskData, err := getDesiredDiskDataForBackup(opts, curExCfg) + pvcDiskData, classicDiskData, err := getDesiredDiskDataForBackup(opts) if err != nil { opts.VMCtx.Logger.Error(err, "failed to get disk data for backup") return err } - if pvcDiskData == "" { + curPvcDiskData, _ := curExCfg.GetString(backupapi.PVCDiskDataExtraConfigKey) + if pvcDiskData == curPvcDiskData { opts.VMCtx.Logger.V(4).Info("Skipping PVC disk data backup as unchanged") } else { ecToUpdate = append(ecToUpdate, &vimtypes.OptionValue{ @@ -133,7 +134,8 @@ func BackupVirtualMachine(opts BackupVirtualMachineOptions) (result error) { } if pkgcfg.FromContext(opts.VMCtx).Features.VMIncrementalRestore { - if classicDiskData == "" { + curClassDiskData, _ := curExCfg.GetString(backupapi.ClassicDiskDataExtraConfigKey) + if classicDiskData == curClassDiskData { opts.VMCtx.Logger.V(4).Info("Skipping classic disk data backup as unchanged") } else { ecToUpdate = append(ecToUpdate, &vimtypes.OptionValue{ @@ -240,20 +242,20 @@ func canPerformBackup(opts BackupVirtualMachineOptions, backupVersionEcVal, back storedVersion, err := strconv.ParseInt(backupVersionEcVal, 10, 64) if err != nil { - opts.VMCtx.Logger.Error(err, fmt.Sprintf("failed to parse backup version extraConfig key to int64 : %v", backupVersionEcVal)) + opts.VMCtx.Logger.Error(err, "failed to parse backup version ExtraConfig value to int64", "version", backupVersionEcVal) return false, err } currVersion, err := strconv.ParseInt(backupVersionAnnotation, 10, 64) if err != nil { - opts.VMCtx.Logger.Error(err, fmt.Sprintf("failed to parse backup version annotation to int64: %v", backupVersionAnnotation)) + opts.VMCtx.Logger.Error(err, "failed to parse backup version annotation value to int64", "version", backupVersionAnnotation) return false, err } - // if the storedVersion in the backup is older than the annotation, this is a restore. + // If the storedVersion in the backup is older than the annotation, this is a restore. if currVersion > storedVersion { - opts.VMCtx.Logger.Info(fmt.Sprintf("Skipping VM backup as a restore is detected: current version: %s greater than backup extraconfig version: %s, wait for VM registration", - backupVersionAnnotation, backupVersionEcVal)) + opts.VMCtx.Logger.Info("Skipping VM backup because restore is detected: current version is greater than backup ExtraConfig version. Wait for VM registration", + "currentVersion", backupVersionAnnotation, "extraConfigVersion", backupVersionEcVal) return false, nil } @@ -401,14 +403,7 @@ func getBackupResourceVersions(ecResourceData string) (map[string]string, error) return resourceVersions, nil } -func getDesiredDiskDataForBackup( - opts BackupVirtualMachineOptions, - extraConfig pkgutil.OptionValues) (string, string, error) { - - // Return an empty string to skip backup if no disk data is specified. - if len(opts.DiskUUIDToPVC) == 0 && len(opts.ClassicDiskUUIDs) == 0 { - return "", "", nil - } +func getDesiredDiskDataForBackup(opts BackupVirtualMachineOptions) (string, string, error) { deviceList, err := opts.VcVM.Device(opts.VMCtx) if err != nil { @@ -438,36 +433,29 @@ func getDesiredDiskDataForBackup( } } - pvcDiskDataJSON, err := json.Marshal(pvcDiskData) - if err != nil { - return "", "", err - } - pvcDiskDataBackup, err := pkgutil.EncodeGzipBase64(string(pvcDiskDataJSON)) - if err != nil { - return "", "", err - } - - // Return an empty string to skip backup if PVC disk data is unchanged. - curBackup, _ := extraConfig.GetString(backupapi.PVCDiskDataExtraConfigKey) - if pvcDiskDataBackup == curBackup { - pvcDiskDataBackup = "" - } - - var classicDiskDataBackup string - if pkgcfg.FromContext(opts.VMCtx).Features.VMIncrementalRestore { - classicDiskDataJSON, err := json.Marshal(classicDiskData) + var pvcDiskDataBackup string + if len(pvcDiskData) > 0 { + pvcDiskDataJSON, err := json.Marshal(pvcDiskData) if err != nil { return "", "", err } - classicDiskDataBackup, err = pkgutil.EncodeGzipBase64(string(classicDiskDataJSON)) + pvcDiskDataBackup, err = pkgutil.EncodeGzipBase64(string(pvcDiskDataJSON)) if err != nil { return "", "", err } + } - // Return an empty string to skip backup if classic disk data is unchanged. - curBackup, _ := extraConfig.GetString(backupapi.ClassicDiskDataExtraConfigKey) - if classicDiskDataBackup == curBackup { - classicDiskDataBackup = "" + var classicDiskDataBackup string + if len(classicDiskData) > 0 { + if pkgcfg.FromContext(opts.VMCtx).Features.VMIncrementalRestore { + classicDiskDataJSON, err := json.Marshal(classicDiskData) + if err != nil { + return "", "", err + } + classicDiskDataBackup, err = pkgutil.EncodeGzipBase64(string(classicDiskDataJSON)) + if err != nil { + return "", "", err + } } } diff --git a/pkg/providers/vsphere/virtualmachine/backup_test.go b/pkg/providers/vsphere/virtualmachine/backup_test.go index bb60ba137..bde428b6b 100644 --- a/pkg/providers/vsphere/virtualmachine/backup_test.go +++ b/pkg/providers/vsphere/virtualmachine/backup_test.go @@ -692,6 +692,40 @@ func backupTests() { Expect(vmCtx.VM.Annotations[vmopv1.VirtualMachineBackupVersionAnnotation]).To(Equal(vT2)) } }) + + It("Should clear PVC disk data in ExtraConfig when no PVCs", func() { + dummyPVC := builder.DummyPersistentVolumeClaim() + diskUUIDToPVC := map[string]corev1.PersistentVolumeClaim{ + vcSimDiskUUID: *dummyPVC, + } + + backupOpts := virtualmachine.BackupVirtualMachineOptions{ + VMCtx: vmCtx, + VcVM: vcVM, + DiskUUIDToPVC: diskUUIDToPVC, + } + + if IncrementalRestore { + backupOpts.BackupVersion = vT2 + } + + Expect(virtualmachine.BackupVirtualMachine(backupOpts)).To(Succeed()) + + backupOpts.DiskUUIDToPVC = nil + + if IncrementalRestore { + backupOpts.BackupVersion = vT3 + } + + Expect(virtualmachine.BackupVirtualMachine(backupOpts)).To(Succeed()) + + verifyBackupDataInExtraConfig(ctx, vcVM, backupapi.PVCDiskDataExtraConfigKey, "", true) + + if IncrementalRestore { + verifyBackupDataInExtraConfig(ctx, vcVM, backupapi.BackupVersionExtraConfigKey, vT3, false) + Expect(vmCtx.VM.Annotations[vmopv1.VirtualMachineBackupVersionAnnotation]).To(Equal(vT3)) + } + }) }) }) @@ -1001,6 +1035,33 @@ func backupTests() { Expect(vmCtx.VM.Annotations[vmopv1.VirtualMachineBackupVersionAnnotation]).To(Equal(vT1)) }) }) + + When("VM already has classic disk data backup in ExtraConfig", func() { + JustBeforeEach(func() { + backupOpts := virtualmachine.BackupVirtualMachineOptions{ + VMCtx: vmCtx, + VcVM: vcVM, + ClassicDiskUUIDs: map[string]struct{}{vcSimDiskUUID: {}}, + BackupVersion: vT1, + } + Expect(virtualmachine.BackupVirtualMachine(backupOpts)).To(Succeed()) + }) + + It("Should clear classic disk data backup if no classic disks in ExtraConfig", func() { + backupOpts := virtualmachine.BackupVirtualMachineOptions{ + VMCtx: vmCtx, + VcVM: vcVM, + ClassicDiskUUIDs: nil, + BackupVersion: vT2, + } + Expect(virtualmachine.BackupVirtualMachine(backupOpts)).To(Succeed()) + + verifyBackupDataInExtraConfig(ctx, vcVM, backupapi.ClassicDiskDataExtraConfigKey, "", true) + + verifyBackupDataInExtraConfig(ctx, vcVM, backupapi.BackupVersionExtraConfigKey, vT2, false) + Expect(vmCtx.VM.Annotations[vmopv1.VirtualMachineBackupVersionAnnotation]).To(Equal(vT2)) + }) + }) }) } },