From bb1c88106021e703039cde93a563a841ec9d7157 Mon Sep 17 00:00:00 2001
From: Bryan Venteicher <bryan.venteicher@broadcom.com>
Date: Thu, 30 Jan 2025 14:13:59 -0600
Subject: [PATCH] Fix duplicate append of Image.Status fields

Add back the content version check and Disks not empty hack. This will
prevent duplicate CL task from being created when the CL item has not
changed.

Clear arrays in the Image Status before updating them. This bug had been
mostly masked by the short circuit check above but without clearing it
the duplicate appends would eventually cause the object to exceed the
allowed limit.
---
 .../utils/controller_builder.go               |  6 ++++
 .../utils/controller_builder_test.go          | 34 +++++++++++++++++++
 .../contentlibrary/content_library_utils.go   |  4 +++
 .../content_library_utils_test.go             | 10 ++++++
 4 files changed, 54 insertions(+)

diff --git a/controllers/contentlibrary/utils/controller_builder.go b/controllers/contentlibrary/utils/controller_builder.go
index e228c2249..ca3043fac 100644
--- a/controllers/contentlibrary/utils/controller_builder.go
+++ b/controllers/contentlibrary/utils/controller_builder.go
@@ -462,6 +462,12 @@ func (r *Reconciler) syncImageContent(
 	vmiStatus *vmopv1.VirtualMachineImageStatus) error {
 
 	latestVersion := cliStatus.ContentVersion
+	if vmiStatus.ProviderContentVersion == latestVersion {
+		// Hack: populate Disks fields during version upgrade.
+		if len(vmiStatus.Disks) != 0 {
+			return nil
+		}
+	}
 
 	err := r.VMProvider.SyncVirtualMachineImage(ctx, cliObj, vmiObj)
 	if err != nil {
diff --git a/controllers/contentlibrary/utils/controller_builder_test.go b/controllers/contentlibrary/utils/controller_builder_test.go
index c38ea07b1..68b55291e 100644
--- a/controllers/contentlibrary/utils/controller_builder_test.go
+++ b/controllers/contentlibrary/utils/controller_builder_test.go
@@ -433,6 +433,40 @@ var _ = Describe("Reconcile",
 						})
 					})
 				})
+
+				When("Image resource is created and already up-to-date and Status.Disk is not empty", func() {
+
+					JustBeforeEach(func() {
+						newVMI(
+							ctx,
+							req.Namespace,
+							vmiName,
+							vmopv1.VirtualMachineImageStatus{
+								ProviderContentVersion: cliStatus.ContentVersion,
+								Disks:                  make([]vmopv1.VirtualMachineImageDiskInfo, 1),
+								Firmware:               "should-not-be-updated",
+							})
+					})
+
+					BeforeEach(func() {
+						fakeVMProvider.SyncVirtualMachineImageFn = func(_ context.Context, _, vmiObj client.Object) error {
+							// This should not be called since the content versions match and disks isn't empty.
+							vmi := vmiObj.(*vmopv1.VirtualMachineImage)
+							vmi.Status.Firmware = firmwareValue
+							return fmt.Errorf("sync-error")
+						}
+					})
+
+					It("should skip updating the ClusterVirtualMachineImage with library item", func() {
+						_, err := reconciler.Reconcile(context.Background(), req)
+						Expect(err).ToNot(HaveOccurred())
+						cliObj, cliSpec, cliStatus = getCLI(ctx, req.Namespace, req.Name)
+
+						vmiObj, vmiSpec, vmiStatus := getVMI(ctx, req.Namespace, vmiName)
+						assertVMImageFromCLItem(cliObj, *cliSpec, *cliStatus, vmiObj, *vmiSpec, *vmiStatus)
+						Expect(vmiStatus.Firmware).To(Equal("should-not-be-updated"))
+					})
+				})
 			})
 
 			Context("ReconcileDelete", func() {
diff --git a/pkg/providers/vsphere/contentlibrary/content_library_utils.go b/pkg/providers/vsphere/contentlibrary/content_library_utils.go
index 140fae97b..97f7ae198 100644
--- a/pkg/providers/vsphere/contentlibrary/content_library_utils.go
+++ b/pkg/providers/vsphere/contentlibrary/content_library_utils.go
@@ -65,6 +65,8 @@ func UpdateVmiWithOvfEnvelope(obj client.Object, ovfEnvelope ovf.Envelope) {
 
 	if ovfEnvelope.Disk != nil && len(ovfEnvelope.Disk.Disks) > 0 {
 		populateImageStatusFromOVFDiskSection(status, ovfEnvelope.Disk)
+	} else {
+		status.Disks = nil
 	}
 }
 
@@ -107,6 +109,7 @@ func initImageStatusFromOVFVirtualSystem(
 		}
 	}
 
+	imageStatus.OVFProperties = nil
 	for _, product := range ovfVirtualSystem.Product {
 		for _, prop := range product.Property {
 			// Only show user configurable properties
@@ -121,6 +124,7 @@ func initImageStatusFromOVFVirtualSystem(
 		}
 	}
 
+	imageStatus.VMwareSystemProperties = nil
 	ovfSystemProps := getVmwareSystemPropertiesFromOvf(ovfVirtualSystem)
 	if len(ovfSystemProps) > 0 {
 		for k, v := range ovfSystemProps {
diff --git a/pkg/providers/vsphere/contentlibrary/content_library_utils_test.go b/pkg/providers/vsphere/contentlibrary/content_library_utils_test.go
index 44feab8da..38605bdb2 100644
--- a/pkg/providers/vsphere/contentlibrary/content_library_utils_test.go
+++ b/pkg/providers/vsphere/contentlibrary/content_library_utils_test.go
@@ -177,6 +177,16 @@ var _ = Describe("UpdateVmiWithOvfEnvelope", func() {
 		Expect(image.Status.Disks[1].Capacity.String()).To(Equal("10Gi"))
 	})
 
+	It("Repeated UpdateVmiWithOvfEnvelope should not duplicated items", func() {
+		Expect(image.Status.Disks).ToNot(BeEmpty())
+		Expect(image.Status.OVFProperties).ToNot(BeEmpty())
+		Expect(image.Status.VMwareSystemProperties).ToNot(BeEmpty())
+
+		savedImage := image.DeepCopy()
+		contentlibrary.UpdateVmiWithOvfEnvelope(image, ovfEnvelope)
+		Expect(image).To(Equal(savedImage))
+	})
+
 	Context("Image is V1Alpha1Compatible", func() {
 		BeforeEach(func() {
 			ovfEnvelope.VirtualSystem.VirtualHardware[0].ExtraConfig = append(ovfEnvelope.VirtualSystem.VirtualHardware[0].ExtraConfig,