Skip to content

Commit

Permalink
Fix duplicate append of Image.Status fields
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Bryan Venteicher committed Jan 30, 2025
1 parent a0cae7a commit bb1c881
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 0 deletions.
6 changes: 6 additions & 0 deletions controllers/contentlibrary/utils/controller_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
34 changes: 34 additions & 0 deletions controllers/contentlibrary/utils/controller_builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
4 changes: 4 additions & 0 deletions pkg/providers/vsphere/contentlibrary/content_library_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down Expand Up @@ -107,6 +109,7 @@ func initImageStatusFromOVFVirtualSystem(
}
}

imageStatus.OVFProperties = nil
for _, product := range ovfVirtualSystem.Product {
for _, prop := range product.Property {
// Only show user configurable properties
Expand All @@ -121,6 +124,7 @@ func initImageStatusFromOVFVirtualSystem(
}
}

imageStatus.VMwareSystemProperties = nil
ovfSystemProps := getVmwareSystemPropertiesFromOvf(ovfVirtualSystem)
if len(ovfSystemProps) > 0 {
for k, v := range ovfSystemProps {
Expand Down
10 changes: 10 additions & 0 deletions pkg/providers/vsphere/contentlibrary/content_library_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit bb1c881

Please sign in to comment.