Address review comments

controllers/virtualmachine/v1alpha1/virtualmachine_controller.go

@@ -474,8 +474,7 @@ func (r *Reconciler) ReconcileNormal(ctx *context.VirtualMachineContext) (reterr
 
 	// Back up this VM if ReconcileNormal succeeds and the FSS is enabled.
 	if lib.IsVMServiceBackupRestoreFSSEnabled() {
-		err := r.VMProvider.BackupVirtualMachine(ctx, ctx.VM)
-		if err != nil {
+		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

controllers/virtualmachine/v1alpha1/virtualmachine_controller_unit_test.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"errors"
 	"strings"
+	"sync/atomic"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -158,18 +159,24 @@ func unitTestsReconcile() {
 
 		When("The VM Service Backup and Restore FSS is enabled", func() {
 			var (
-				originalIsVMServiceBackupRestoreFSSEnabled func() bool
+				// This is manipulated atomically to avoid races where the controller
+				// is trying to read this _while_ the tests are updating it.
+				vmServiceBackupRestoreFSS     uint32
+				origVMServiceBackupRestoreFSS uint32
 			)
 
+			// Modify the helper function to return the custom value of the FSS.
+			lib.IsVMServiceBackupRestoreFSSEnabled = func() bool {
+				return atomic.LoadUint32(&vmServiceBackupRestoreFSS) != 0
+			}
+
 			BeforeEach(func() {
-				originalIsVMServiceBackupRestoreFSSEnabled = lib.IsVMServiceBackupRestoreFSSEnabled
-				lib.IsVMServiceBackupRestoreFSSEnabled = func() bool {
-					return true
-				}
+				origVMServiceBackupRestoreFSS = vmServiceBackupRestoreFSS
+				atomic.StoreUint32(&vmServiceBackupRestoreFSS, 1)
 			})
 
 			AfterEach(func() {
-				lib.IsVMServiceBackupRestoreFSSEnabled = originalIsVMServiceBackupRestoreFSSEnabled
+				atomic.StoreUint32(&vmServiceBackupRestoreFSS, origVMServiceBackupRestoreFSS)
 			})
 
 			It("Should call backup Virtual Machine if ReconcileNormal succeeds", func() {

pkg/util/enc.go

@@ -64,6 +64,7 @@ func EncodeGzipBase64(s string) (string, error) {
 	if _, err := zw.Write([]byte(s)); err != nil {
 		return "", err
 	}
+	// Flush before closing to ensure all data is written.
 	if err := zw.Flush(); err != nil {
 		return "", err
 	}

pkg/util/enc_test.go

@@ -7,6 +7,7 @@ import (
 	"bytes"
 	"compress/gzip"
 	"encoding/base64"
+	"io/ioutil"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -159,7 +160,18 @@ var _ = Describe("EncodeGzipBase64", func() {
 	It("Encodes a string correctly", func() {
 		input := "HelloWorld"
 		output, err := util.EncodeGzipBase64(input)
-		Expect(err).ShouldNot(HaveOccurred())
-		Expect(output).To(Equal("H4sIAAAAAAAA//JIzcnJD88vykkBAAAA//8BAAD//3kMd3cKAAAA"))
+		Expect(err).NotTo(HaveOccurred())
+		// Decode the base64 output.
+		decoded, err := base64.StdEncoding.DecodeString(output)
+		Expect(err).NotTo(HaveOccurred())
+		// Ungzip the decoded output.
+		reader := bytes.NewReader(decoded)
+		gzipReader, err := gzip.NewReader(reader)
+		Expect(err).NotTo(HaveOccurred())
+		defer Expect(gzipReader.Close()).To(Succeed())
+
+		ungzipped, err := ioutil.ReadAll(gzipReader)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(input).Should(Equal(string(ungzipped)))
 	})
 })

pkg/vmprovider/providers/vsphere/virtualmachine/backup.go

@@ -4,21 +4,22 @@
 package virtualmachine
 
 import (
-	goctx "context"
-
-	"github.com/vmware/govmomi/object"
-	"github.com/vmware/govmomi/vim25/types"
 	corev1 "k8s.io/api/core/v1"
 	ctrlruntime "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/yaml"
 
+	"github.com/vmware/govmomi/object"
+	"github.com/vmware/govmomi/vim25/types"
+
 	vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha1"
 	"github.com/vmware-tanzu/vm-operator/pkg/context"
 	"github.com/vmware-tanzu/vm-operator/pkg/util"
 	"github.com/vmware-tanzu/vm-operator/pkg/vmprovider/providers/vsphere/constants"
 )
 
-// BackupVirtualMachine backs up the VM's kube data and bootstrap data into the VM's ExtraConfig.
+// BackupVirtualMachine backs up the VM's kube data and bootstrap data into the
+// vSphere VM's ExtraConfig. The status fields will not be backed up, as we expect
+// to recreate and reconcile these resources during the restore process.
 func BackupVirtualMachine(
 	vmCtx context.VirtualMachineContext,
 	vcVM *object.VirtualMachine,
@@ -29,7 +30,7 @@ func BackupVirtualMachine(
 		return err
 	}
 
-	vmBootstrapData, err := getEncodedVMBootstrapData(vmCtx.Context, vmCtx.VM, k8sClient)
+	vmBootstrapData, err := getEncodedVMBootstrapData(vmCtx, k8sClient)
 	if err != nil {
 		vmCtx.Logger.Error(err, "Failed to get VM bootstrap data for backup")
 		return err
@@ -59,7 +60,6 @@ func BackupVirtualMachine(
 
 func getEncodedVMKubeData(vm *vmopv1.VirtualMachine) (string, error) {
 	backupVM := vm.DeepCopy()
-	// No need to back up status fields as we expect the VM to be re-created during restore.
 	backupVM.Status = vmopv1.VirtualMachineStatus{}
 	backupYaml, err := yaml.Marshal(backupVM)
 	if err != nil {
@@ -70,31 +70,36 @@ func getEncodedVMKubeData(vm *vmopv1.VirtualMachine) (string, error) {
 }
 
 func getEncodedVMBootstrapData(
-	ctx goctx.Context,
-	vm *vmopv1.VirtualMachine,
+	vmCtx context.VirtualMachineContext,
 	k8sClient ctrlruntime.Client) (string, error) {
-	// Return early if the VM bootstrap data is not set.
-	if vm.Spec.VmMetadata == nil {
+	k8sVM := vmCtx.VM
+	if k8sVM.Spec.VmMetadata == nil {
 		return "", nil
 	}
 
 	var bootstrapObj ctrlruntime.Object
 
-	if cmName := vm.Spec.VmMetadata.ConfigMapName; cmName != "" {
+	if cmName := k8sVM.Spec.VmMetadata.ConfigMapName; cmName != "" {
 		cm := &corev1.ConfigMap{}
-		cmKey := ctrlruntime.ObjectKey{Name: cmName, Namespace: vm.Namespace}
-		if err := k8sClient.Get(ctx, cmKey, cm); err != nil {
+		cmKey := ctrlruntime.ObjectKey{
+			Name:      cmName,
+			Namespace: k8sVM.Namespace,
+		}
+		if err := k8sClient.Get(vmCtx.Context, cmKey, cm); err != nil {
 			return "", err
 		}
 		cm.APIVersion = "v1"
 		cm.Kind = "ConfigMap"
 		bootstrapObj = cm
 	}
 
-	if secretName := vm.Spec.VmMetadata.SecretName; secretName != "" {
+	if secretName := k8sVM.Spec.VmMetadata.SecretName; secretName != "" {
 		secret := &corev1.Secret{}
-		secretKey := ctrlruntime.ObjectKey{Name: secretName, Namespace: vm.Namespace}
-		if err := k8sClient.Get(ctx, secretKey, secret); err != nil {
+		secretKey := ctrlruntime.ObjectKey{
+			Name:      secretName,
+			Namespace: k8sVM.Namespace,
+		}
+		if err := k8sClient.Get(vmCtx.Context, secretKey, secret); err != nil {
 			return "", err
 		}
 		secret.APIVersion = "v1"

pkg/vmprovider/providers/vsphere/virtualmachine/backup_test.go

@@ -4,15 +4,16 @@
 package virtualmachine_test
 
 import (
-	. "github.com/onsi/ginkgo"
-	. "github.com/onsi/gomega"
-	"github.com/vmware/govmomi/object"
-	"github.com/vmware/govmomi/vim25/mo"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	ctrlruntime "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/yaml"
 
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/vmware/govmomi/object"
+	"github.com/vmware/govmomi/vim25/mo"
+
 	vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha1"
 	"github.com/vmware-tanzu/vm-operator/pkg/context"
 	"github.com/vmware-tanzu/vm-operator/pkg/util"