✨ Update VM backup to skip unchanged data and include instance ID (#239)

This patch includes the following changes for the VM backup & restore workflow:
- Move ExtraConfigToMap and MergeExtraConfig util functions to "pkg/util/configspec.go"
- Persist cloud-init instance ID to keep it unchanged upon VM restore.
- Optimize backup process to skip reconfiguring the VM with unchanged data.
- Persist backup info in VM's EC after successfully updating the VM object to avoid additional calls from the controller.

controllers/virtualmachine/v1alpha1/virtualmachine_controller.go

@@ -471,15 +471,6 @@ func (r *Reconciler) ReconcileNormal(ctx *context.VirtualMachineContext) (reterr
 	// Add this VM to prober manager if ReconcileNormal succeeds.
 	r.Prober.AddToProberManager(ctx.VM)
 
-	// Back up this VM if ReconcileNormal succeeds and the FSS is enabled.
-	if lib.IsVMServiceBackupRestoreFSSEnabled() {
-		if err := r.VMProvider.BackupVirtualMachine(ctx, ctx.VM); err != nil {
-			ctx.Logger.Error(err, "Failed to backup VirtualMachine")
-			r.Recorder.EmitEvent(ctx.VM, "Backup", err, false)
-			return err
-		}
-	}
-
 	ctx.Logger.Info("Finished Reconciling VirtualMachine")
 	return nil
 }

controllers/virtualmachine/v1alpha1/virtualmachine_controller_unit_test.go

@@ -6,7 +6,6 @@ package v1alpha1_test
 import (
 	"context"
 	"errors"
-	"os"
 	"strings"
 
 	. "github.com/onsi/ginkgo"
@@ -19,7 +18,6 @@ import (
 
 	virtualmachine "github.com/vmware-tanzu/vm-operator/controllers/virtualmachine/v1alpha1"
 	vmopContext "github.com/vmware-tanzu/vm-operator/pkg/context"
-	"github.com/vmware-tanzu/vm-operator/pkg/lib"
 	proberfake "github.com/vmware-tanzu/vm-operator/pkg/prober/fake"
 	providerfake "github.com/vmware-tanzu/vm-operator/pkg/vmprovider/fake"
 	"github.com/vmware-tanzu/vm-operator/test/builder"
@@ -156,27 +154,6 @@ func unitTestsReconcile() {
 			Expect(reconciler.ReconcileNormal(vmCtx)).Should(Succeed())
 			Expect(fakeProbeManager.IsAddToProberManagerCalled).Should(BeTrue())
 		})
-
-		When("The VM Service Backup and Restore FSS is enabled", func() {
-			BeforeEach(func() {
-				Expect(os.Setenv(lib.VMServiceBackupRestoreFSS, lib.TrueString)).To(Succeed())
-			})
-
-			AfterEach(func() {
-				Expect(os.Unsetenv(lib.VMServiceBackupRestoreFSS)).To(Succeed())
-			})
-
-			It("Should call backup Virtual Machine if ReconcileNormal succeeds", func() {
-				var isBackupVirtualMachineCalled bool
-				fakeVMProvider.BackupVirtualMachineFn = func(ctx context.Context, vm *vmopv1.VirtualMachine) error {
-					isBackupVirtualMachineCalled = true
-					return nil
-				}
-
-				Expect(reconciler.ReconcileNormal(vmCtx)).Should(Succeed())
-				Expect(isBackupVirtualMachineCalled).Should(BeTrue())
-			})
-		})
 	})
 
 	Context("ReconcileDelete", func() {

pkg/util/configspec.go

@@ -188,3 +188,30 @@ func AppendNewExtraConfigValues(
 
 	return append(extraConfig, newExtraConfig...)
 }
+
+// ExtraConfigToMap converts the ExtraConfig to a map with string values.
+func ExtraConfigToMap(input []vimTypes.BaseOptionValue) (output map[string]string) {
+	output = make(map[string]string)
+	for _, opt := range input {
+		if optValue := opt.GetOptionValue(); optValue != nil {
+			// Only set string type values
+			if val, ok := optValue.Value.(string); ok {
+				output[optValue.Key] = val
+			}
+		}
+	}
+	return
+}
+
+// MergeExtraConfig adds the key/value to the ExtraConfig if the key is not present.
+// It returns the newly added ExtraConfig.
+func MergeExtraConfig(extraConfig []vimTypes.BaseOptionValue, newMap map[string]string) []vimTypes.BaseOptionValue {
+	merged := make([]vimTypes.BaseOptionValue, 0)
+	ecMap := ExtraConfigToMap(extraConfig)
+	for k, v := range newMap {
+		if _, exists := ecMap[k]; !exists {
+			merged = append(merged, &vimTypes.OptionValue{Key: k, Value: v})
+		}
+	}
+	return merged
+}

pkg/util/configspec_test.go

@@ -386,6 +386,83 @@ var _ = Describe("SanitizeVMClassConfigSpec", func() {
 	})
 })
 
+var _ = Describe("ExtraConfigToMap", func() {
+	var (
+		extraConfig    []vimTypes.BaseOptionValue
+		extraConfigMap map[string]string
+	)
+	BeforeEach(func() {
+		extraConfig = []vimTypes.BaseOptionValue{}
+	})
+	JustBeforeEach(func() {
+		extraConfigMap = util.ExtraConfigToMap(extraConfig)
+	})
+
+	Context("Empty extraConfig", func() {
+		It("Return empty map", func() {
+			Expect(extraConfigMap).To(HaveLen(0))
+		})
+	})
+
+	Context("With extraConfig", func() {
+		BeforeEach(func() {
+			extraConfig = append(extraConfig, &vimTypes.OptionValue{Key: "key1", Value: "value1"})
+			extraConfig = append(extraConfig, &vimTypes.OptionValue{Key: "key2", Value: "value2"})
+		})
+		It("Return valid map", func() {
+			Expect(extraConfigMap).To(HaveLen(2))
+			Expect(extraConfigMap["key1"]).To(Equal("value1"))
+			Expect(extraConfigMap["key2"]).To(Equal("value2"))
+		})
+	})
+})
+
+var _ = Describe("MergeExtraConfig", func() {
+	var (
+		extraConfig []vimTypes.BaseOptionValue
+		newMap      map[string]string
+		merged      []vimTypes.BaseOptionValue
+	)
+	BeforeEach(func() {
+		extraConfig = []vimTypes.BaseOptionValue{
+			&vimTypes.OptionValue{Key: "existingkey1", Value: "existingvalue1"},
+			&vimTypes.OptionValue{Key: "existingkey2", Value: "existingvalue2"},
+		}
+		newMap = map[string]string{}
+	})
+	JustBeforeEach(func() {
+		merged = util.MergeExtraConfig(extraConfig, newMap)
+	})
+
+	Context("Empty newMap", func() {
+		It("Return empty merged", func() {
+			Expect(merged).To(BeEmpty())
+		})
+	})
+
+	Context("NewMap with existing key", func() {
+		BeforeEach(func() {
+			newMap["existingkey1"] = "existingkey1"
+		})
+		It("Return empty merged", func() {
+			Expect(merged).To(BeEmpty())
+		})
+	})
+
+	Context("NewMap with new keys", func() {
+		BeforeEach(func() {
+			newMap["newkey1"] = "newvalue1"
+			newMap["newkey2"] = "newvalue2"
+		})
+		It("Return merged map", func() {
+			Expect(merged).To(HaveLen(2))
+			mergedMap := util.ExtraConfigToMap(merged)
+			Expect(mergedMap["newkey1"]).To(Equal("newvalue1"))
+			Expect(mergedMap["newkey2"]).To(Equal("newvalue2"))
+		})
+	})
+})
+
 func mustParseTime(layout, value string) time.Time {
 	t, err := time.Parse(layout, value)
 	if err != nil {

pkg/vmprovider/fake/fake_vm_provider.go

@@ -31,7 +31,6 @@ type funcs struct {
 	DeleteVirtualMachineFn         func(ctx context.Context, vm *vmopv1.VirtualMachine) error
 	PublishVirtualMachineFn        func(ctx context.Context, vm *vmopv1.VirtualMachine,
 		vmPub *vmopv1.VirtualMachinePublishRequest, cl *imgregv1a1.ContentLibrary, actID string) (string, error)
-	BackupVirtualMachineFn             func(ctx context.Context, vm *vmopv1.VirtualMachine) error
 	GetVirtualMachineGuestHeartbeatFn  func(ctx context.Context, vm *vmopv1.VirtualMachine) (vmopv1.GuestHeartbeatStatus, error)
 	GetVirtualMachineWebMKSTicketFn    func(ctx context.Context, vm *vmopv1.VirtualMachine, pubKey string) (string, error)
 	GetVirtualMachineHardwareVersionFn func(ctx context.Context, vm *vmopv1.VirtualMachine) (int32, error)
@@ -113,17 +112,6 @@ func (s *VMProvider) PublishVirtualMachine(ctx context.Context, vm *vmopv1.Virtu
 	return "dummy-id", nil
 }
 
-func (s *VMProvider) BackupVirtualMachine(ctx context.Context, vm *vmopv1.VirtualMachine) error {
-	s.Lock()
-	defer s.Unlock()
-
-	if s.BackupVirtualMachineFn != nil {
-		return s.BackupVirtualMachineFn(ctx, vm)
-	}
-
-	return nil
-}
-
 func (s *VMProvider) GetVirtualMachineGuestHeartbeat(ctx context.Context, vm *vmopv1.VirtualMachine) (vmopv1.GuestHeartbeatStatus, error) {
 	s.Lock()
 	defer s.Unlock()

pkg/vmprovider/interface.go

@@ -22,7 +22,6 @@ type VirtualMachineProviderInterface interface {
 	DeleteVirtualMachine(ctx context.Context, vm *vmopv1.VirtualMachine) error
 	PublishVirtualMachine(ctx context.Context, vm *vmopv1.VirtualMachine,
 		vmPub *vmopv1.VirtualMachinePublishRequest, cl *imgregv1a1.ContentLibrary, actID string) (string, error)
-	BackupVirtualMachine(ctx context.Context, vm *vmopv1.VirtualMachine) error
 	GetVirtualMachineGuestHeartbeat(ctx context.Context, vm *vmopv1.VirtualMachine) (vmopv1.GuestHeartbeatStatus, error)
 	GetVirtualMachineWebMKSTicket(ctx context.Context, vm *vmopv1.VirtualMachine, pubKey string) (string, error)
 	GetVirtualMachineHardwareVersion(ctx context.Context, vm *vmopv1.VirtualMachine) (int32, error)

pkg/vmprovider/providers/vsphere/constants/constants.go

@@ -128,4 +128,7 @@ const (
 	// BackupVMDiskDataExtraConfigKey is the ExtraConfig key to the VM's disk info
 	// data in JSON, compressed using gzip and base64-encoded.
 	BackupVMDiskDataExtraConfigKey = "vmservice.virtualmachine.diskdata"
+	// BackupVMCloudInitInstanceIDExtraConfigKey is the ExtraConfig key to the VM's
+	// Cloud-Init instance ID, compressed using gzip and base64-encoded.
+	BackupVMCloudInitInstanceIDExtraConfigKey = "vmservice.virtualmachine.cloudinit.instanceid"
 )

pkg/vmprovider/providers/vsphere/session/session_util.go

@@ -13,32 +13,6 @@ const (
 	OvfEnvironmentTransportGuestInfo = "com.vmware.guestInfo"
 )
 
-func ExtraConfigToMap(input []vimTypes.BaseOptionValue) (output map[string]string) {
-	output = make(map[string]string)
-	for _, opt := range input {
-		if optValue := opt.GetOptionValue(); optValue != nil {
-			// Only set string type values
-			if val, ok := optValue.Value.(string); ok {
-				output[optValue.Key] = val
-			}
-		}
-	}
-	return
-}
-
-// MergeExtraConfig adds the key/value to the ExtraConfig if the key is not present, to let to the value be
-// changed by the VM. The existing usage of ExtraConfig is hard to fit in the reconciliation model.
-func MergeExtraConfig(extraConfig []vimTypes.BaseOptionValue, newMap map[string]string) []vimTypes.BaseOptionValue {
-	merged := make([]vimTypes.BaseOptionValue, 0)
-	ecMap := ExtraConfigToMap(extraConfig)
-	for k, v := range newMap {
-		if _, exists := ecMap[k]; !exists {
-			merged = append(merged, &vimTypes.OptionValue{Key: k, Value: v})
-		}
-	}
-	return merged
-}
-
 // GetMergedvAppConfigSpec prepares a vApp VmConfigSpec which will set the vmMetadata supplied key/value fields. Only
 // fields marked userConfigurable and pre-existing on the VM (ie. originated from the OVF Image)
 // will be set, and all others will be ignored.

pkg/vmprovider/providers/vsphere/session/session_util_test.go

@@ -79,81 +79,4 @@ var _ = Describe("Test Session Utils", func() {
 			),
 		)
 	})
-
-	Context("ExtraConfigToMap", func() {
-		var (
-			extraConfig    []vimTypes.BaseOptionValue
-			extraConfigMap map[string]string
-		)
-		BeforeEach(func() {
-			extraConfig = []vimTypes.BaseOptionValue{}
-		})
-		JustBeforeEach(func() {
-			extraConfigMap = session.ExtraConfigToMap(extraConfig)
-		})
-
-		Context("Empty extraConfig", func() {
-			It("Return empty map", func() {
-				Expect(extraConfigMap).To(HaveLen(0))
-			})
-		})
-
-		Context("With extraConfig", func() {
-			BeforeEach(func() {
-				extraConfig = append(extraConfig, &vimTypes.OptionValue{Key: "key1", Value: "value1"})
-				extraConfig = append(extraConfig, &vimTypes.OptionValue{Key: "key2", Value: "value2"})
-			})
-			It("Return valid map", func() {
-				Expect(extraConfigMap).To(HaveLen(2))
-				Expect(extraConfigMap["key1"]).To(Equal("value1"))
-				Expect(extraConfigMap["key2"]).To(Equal("value2"))
-			})
-		})
-	})
-
-	Context("MergeExtraConfig", func() {
-		var (
-			extraConfig []vimTypes.BaseOptionValue
-			newMap      map[string]string
-			merged      []vimTypes.BaseOptionValue
-		)
-		BeforeEach(func() {
-			extraConfig = []vimTypes.BaseOptionValue{
-				&vimTypes.OptionValue{Key: "existingkey1", Value: "existingvalue1"},
-				&vimTypes.OptionValue{Key: "existingkey2", Value: "existingvalue2"},
-			}
-			newMap = map[string]string{}
-		})
-		JustBeforeEach(func() {
-			merged = session.MergeExtraConfig(extraConfig, newMap)
-		})
-
-		Context("Empty newMap", func() {
-			It("Return empty merged", func() {
-				Expect(merged).To(BeEmpty())
-			})
-		})
-
-		Context("NewMap with existing key", func() {
-			BeforeEach(func() {
-				newMap["existingkey1"] = "existingkey1"
-			})
-			It("Return empty merged", func() {
-				Expect(merged).To(BeEmpty())
-			})
-		})
-
-		Context("NewMap with new keys", func() {
-			BeforeEach(func() {
-				newMap["newkey1"] = "newvalue1"
-				newMap["newkey2"] = "newvalue2"
-			})
-			It("Return merged map", func() {
-				Expect(merged).To(HaveLen(2))
-				mergedMap := session.ExtraConfigToMap(merged)
-				Expect(mergedMap["newkey1"]).To(Equal("newvalue1"))
-				Expect(mergedMap["newkey2"]).To(Equal("newvalue2"))
-			})
-		})
-	})
 })

pkg/vmprovider/providers/vsphere/session/session_vm_customization.go

@@ -192,7 +192,7 @@ func GetCloudInitGuestInfoCustSpec(
 	}
 
 	configSpec := &vimTypes.VirtualMachineConfigSpec{}
-	configSpec.ExtraConfig = MergeExtraConfig(config.ExtraConfig, extraConfig)
+	configSpec.ExtraConfig = util.MergeExtraConfig(config.ExtraConfig, extraConfig)
 
 	// Remove the VAppConfig to ensure Cloud-Init inside of the guest does not
 	// activate and prefer the OVF datasource over the VMware datasource.
@@ -217,7 +217,7 @@ func GetExtraConfigCustSpec(
 	}
 
 	configSpec := &vimTypes.VirtualMachineConfigSpec{}
-	configSpec.ExtraConfig = MergeExtraConfig(config.ExtraConfig, extraConfig)
+	configSpec.ExtraConfig = util.MergeExtraConfig(config.ExtraConfig, extraConfig)
 	return configSpec
 }