Skip to content

Commit 3e2fc80

Browse files
zhangheliuzhangheliu
authored
fix: fastDeploy directory cleanup (#1476)
Move defer cleanup registration immediately after directory creation for better error handling. Fix cleanup for non-empty directories on non-TLD datastores by using DeleteDatastoreFile() first, then DeleteDirectory() to clean up namespace mapping.
1 parent 465f813 commit 3e2fc80

File tree

2 files changed

+281
-56
lines changed

2 files changed

+281
-56
lines changed

‎pkg/providers/vsphere/vmlifecycle/create_fastdeploy.go‎

Lines changed: 49 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,55 @@ func fastDeploy(
138138
"vmDirPath", vmDirPath,
139139
"vmPathName", createArgs.ConfigSpec.Files.VmPathName)
140140

141+
// Register cleanup defer immediately after directory creation.
142+
defer func() {
143+
if retErr == nil {
144+
// Do not delete the VM directory if this function succeeded.
145+
return
146+
}
147+
148+
// Use a context that is not cancelled when the parent is, so cleanup
149+
// runs even if the request is cancelled. Preserves the VC opID so
150+
// cleanup is correlated with the failed create in VC logs.
151+
ctx := context.WithoutCancel(vmCtx.Context)
152+
153+
// Delete the VM directory and its contents.
154+
// Always use FileManager.DeleteDatastoreFile() first as it can delete
155+
// non-empty directories recursively. Note that DeleteDirectory() on a
156+
// non-empty directory will return an error, which is why we delete the
157+
// directory contents first using DeleteDatastoreFile().
158+
t, err := fm.DeleteDatastoreFile(ctx, vmDirPath, datacenter)
159+
if err != nil {
160+
retErr = fmt.Errorf(
161+
"failed to call delete api for vm dir %q: %w,%w",
162+
vmDirPath, err, retErr)
163+
return
164+
}
165+
166+
// Wait for the delete call to return.
167+
if err := t.Wait(ctx); err != nil &&
168+
!fault.Is(err, &vimtypes.FileNotFound{}) {
169+
170+
retErr = fmt.Errorf(
171+
"failed to delete vm dir %q: %w,%w",
172+
vmDirPath, err, retErr)
173+
}
174+
175+
// For non-TLD datastores, also clean up the namespace mapping.
176+
if !createArgs.Datastores[0].TopLevelDirectoryCreateSupported {
177+
if err := nm.DeleteDirectory(
178+
ctx,
179+
datacenter,
180+
vmDirUUIDPath); err != nil &&
181+
!fault.Is(err, &vimtypes.FileNotFound{}) {
182+
183+
retErr = fmt.Errorf(
184+
"failed to delete vm dir namespace mapping %q: %w,%w",
185+
vmDirUUIDPath, err, retErr)
186+
}
187+
}
188+
}()
189+
141190
dstDiskPaths := make([]string, len(srcDiskPaths))
142191
for i := 0; i < len(dstDiskPaths); i++ {
143192
dstDiskPaths[i] = fmt.Sprintf("%s/disk-%d.vmdk", vmDirPath, i)
@@ -226,62 +275,6 @@ func fastDeploy(
226275
}
227276
}
228277

229-
// If any error occurs after this point, the newly created VM directory and
230-
// its contents need to be cleaned up.
231-
defer func() {
232-
if retErr == nil {
233-
// Do not delete the VM directory if this function was successful.
234-
return
235-
}
236-
237-
// Use a new context to ensure cleanup happens even if the context
238-
// is cancelled.
239-
ctx := context.Background()
240-
241-
// Delete the VM directory and its contents.
242-
if createArgs.Datastores[0].TopLevelDirectoryCreateSupported {
243-
t, err := fm.DeleteDatastoreFile(ctx, vmDirPath, datacenter)
244-
if err != nil {
245-
err = fmt.Errorf(
246-
"failed to call delete api for vm dir %q: %w",
247-
vmDirPath, err)
248-
if retErr == nil {
249-
retErr = err
250-
} else {
251-
retErr = fmt.Errorf("%w,%w", err, retErr)
252-
}
253-
return
254-
}
255-
256-
// Wait for the delete call to return.
257-
if err := t.Wait(ctx); err != nil {
258-
if !fault.Is(err, &vimtypes.FileNotFound{}) {
259-
err = fmt.Errorf("failed to delete vm dir %q: %w",
260-
vmDirPath, err)
261-
if retErr == nil {
262-
retErr = err
263-
} else {
264-
retErr = fmt.Errorf("%w,%w", err, retErr)
265-
}
266-
}
267-
}
268-
} else if err := nm.DeleteDirectory(
269-
ctx,
270-
datacenter,
271-
vmDirUUIDPath); err != nil {
272-
273-
if !fault.Is(err, &vimtypes.FileNotFound{}) {
274-
err = fmt.Errorf("failed to delete vm dir %q: %w",
275-
vmDirUUIDPath, err)
276-
if retErr == nil {
277-
retErr = err
278-
} else {
279-
retErr = fmt.Errorf("%w,%w", err, retErr)
280-
}
281-
}
282-
}
283-
}()
284-
285278
folder := object.NewFolder(vimClient, vimtypes.ManagedObjectReference{
286279
Type: "Folder",
287280
Value: createArgs.FolderMoID,

‎pkg/providers/vsphere/vmlifecycle/create_fastdeploy_test.go‎

Lines changed: 232 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,232 @@
1+
// © Broadcom. All Rights Reserved.
2+
// The term "Broadcom" refers to Broadcom Inc. and/or its subsidiaries.
3+
// SPDX-License-Identifier: Apache-2.0
4+
5+
package vmlifecycle_test
6+
7+
import (
8+
"errors"
9+
"fmt"
10+
"os"
11+
12+
. "github.com/onsi/ginkgo/v2"
13+
. "github.com/onsi/gomega"
14+
15+
vimtypes "github.com/vmware/govmomi/vim25/types"
16+
17+
corev1 "k8s.io/api/core/v1"
18+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
19+
"k8s.io/apimachinery/pkg/types"
20+
21+
vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha5"
22+
pkgcfg "github.com/vmware-tanzu/vm-operator/pkg/config"
23+
pkgctx "github.com/vmware-tanzu/vm-operator/pkg/context"
24+
"github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/vmlifecycle"
25+
pkgutil "github.com/vmware-tanzu/vm-operator/pkg/util"
26+
"github.com/vmware-tanzu/vm-operator/test/builder"
27+
)
28+
29+
var _ = Describe("createFastDeploy", func() {
30+
31+
var (
32+
ctx *builder.TestContextForVCSim
33+
vmCtx pkgctx.VirtualMachineContext
34+
vm *vmopv1.VirtualMachine
35+
)
36+
37+
BeforeEach(func() {
38+
ctx = suite.NewTestContextForVCSim(builder.VCSimTestConfig{
39+
WithContentLibrary: true,
40+
})
41+
42+
pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) {
43+
config.Features.FastDeploy = true
44+
})
45+
46+
vm = builder.DummyVirtualMachine()
47+
vm.Name = "fastdeploy-cleanup-test"
48+
vm.UID = types.UID("cleanup-test-uid-12345")
49+
vm.Namespace = pkgcfg.FromContext(ctx).PodNamespace
50+
51+
vmCtx = pkgctx.VirtualMachineContext{
52+
Context: ctx,
53+
Logger: suite.GetLogger().WithValues("vmName", vm.Name),
54+
VM: vm,
55+
}
56+
})
57+
58+
AfterEach(func() {
59+
ctx.AfterEach()
60+
ctx = nil
61+
})
62+
63+
Context("when directory is created successfully", func() {
64+
// Note: We only test TLD-supported datastores here. Testing non-TLD datastores
65+
// (vSAN) is limited because vcsim may not enforce the "non-empty directory"
66+
// restriction that real vSphere/vSAN does, so we cannot validate the two-step
67+
// cleanup approach (DeleteDatastoreFile + DeleteDirectory) in the simulator.
68+
Context("with TLD-supported datastore (using FileManager.MakeDirectory)", func() {
69+
It("should cleanup directory when error occurs after directory creation", func() {
70+
// Similar test but for TLD-supported datastores
71+
// This uses FileManager.MakeDirectory instead of CreateDirectory
72+
73+
vmicName := pkgutil.VMIName(ctx.ContentLibraryItemID)
74+
vmic := vmopv1.VirtualMachineImageCache{
75+
ObjectMeta: metav1.ObjectMeta{
76+
Namespace: pkgcfg.FromContext(ctx).PodNamespace,
77+
Name: vmicName,
78+
},
79+
Status: vmopv1.VirtualMachineImageCacheStatus{
80+
OVF: &vmopv1.VirtualMachineImageCacheOVFStatus{
81+
ConfigMapName: vmicName,
82+
ProviderVersion: ctx.ContentLibraryItemVersion,
83+
},
84+
Conditions: []metav1.Condition{
85+
{
86+
Type: vmopv1.VirtualMachineImageCacheConditionHardwareReady,
87+
Status: metav1.ConditionTrue,
88+
},
89+
{
90+
Type: vmopv1.VirtualMachineImageCacheConditionFilesReady,
91+
Status: metav1.ConditionTrue,
92+
},
93+
},
94+
Locations: []vmopv1.VirtualMachineImageCacheLocationStatus{
95+
{
96+
DatacenterID: ctx.Datacenter.Reference().Value,
97+
DatastoreID: ctx.Datastore.Reference().Value,
98+
ProfileID: ctx.StorageProfileID,
99+
Files: []vmopv1.VirtualMachineImageCacheFileStatus{
100+
{
101+
ID: ctx.ContentLibraryItemDiskPath,
102+
Type: vmopv1.VirtualMachineImageCacheFileTypeDisk,
103+
DiskType: vmopv1.VolumeTypeClassic,
104+
},
105+
{
106+
ID: ctx.ContentLibraryItemNVRAMPath,
107+
Type: vmopv1.VirtualMachineImageCacheFileTypeOther,
108+
},
109+
},
110+
Conditions: []metav1.Condition{
111+
{
112+
Type: vmopv1.ReadyConditionType,
113+
Status: metav1.ConditionTrue,
114+
},
115+
},
116+
},
117+
},
118+
},
119+
}
120+
121+
vmicm := corev1.ConfigMap{
122+
ObjectMeta: metav1.ObjectMeta{
123+
Namespace: vmic.Namespace,
124+
Name: vmic.Name,
125+
},
126+
Data: map[string]string{
127+
"value": `<?xml version="1.0" encoding="UTF-8"?>
128+
<Envelope xmlns="http://schemas.dmtf.org/ovf/envelope/1">
129+
<References>
130+
<File ovf:href="disk.vmdk" ovf:id="file1"/>
131+
</References>
132+
<DiskSection>
133+
<Info>Virtual disk information</Info>
134+
<Disk ovf:capacity="1024" ovf:diskId="vmdisk1" ovf:fileRef="file1"/>
135+
</DiskSection>
136+
<VirtualSystem>
137+
<Name>test-vm</Name>
138+
<VirtualHardwareSection>
139+
<System>
140+
<vssd:ElementName>Virtual Hardware Family</vssd:ElementName>
141+
<vssd:InstanceID>0</vssd:InstanceID>
142+
<vssd:VirtualSystemIdentifier>test-vm</vssd:VirtualSystemIdentifier>
143+
<vssd:VirtualSystemType>vmx-11</vssd:VirtualSystemType>
144+
</System>
145+
</VirtualHardwareSection>
146+
</VirtualSystem>
147+
</Envelope>`,
148+
},
149+
}
150+
151+
Expect(ctx.Client.Create(ctx, &vmic)).To(Succeed())
152+
Expect(ctx.Client.Create(ctx, &vmicm)).To(Succeed())
153+
Expect(ctx.Client.Status().Update(ctx, &vmic)).To(Succeed())
154+
155+
vmDirName := string(vm.UID)
156+
vmDirPath := fmt.Sprintf("[%s] %s", ctx.Datastore.Name(), vmDirName)
157+
158+
// Verify directory doesn't exist initially
159+
// DatastoreFileExists returns nil when file exists, os.ErrNotExist when it doesn't
160+
err := pkgutil.DatastoreFileExists(ctx, ctx.VCClient.Client, vmDirPath, ctx.Datacenter)
161+
Expect(errors.Is(err, os.ErrNotExist)).To(BeTrue(), "Directory should not exist before creation")
162+
163+
// Create ConfigSpec with 2 disks (mismatch)
164+
configSpec := vimtypes.VirtualMachineConfigSpec{
165+
Name: vm.Name,
166+
Files: &vimtypes.VirtualMachineFileInfo{
167+
VmPathName: fmt.Sprintf("[%s] %s/%s.vmx", ctx.Datastore.Name(), vmDirName, vm.Name),
168+
},
169+
DeviceChange: []vimtypes.BaseVirtualDeviceConfigSpec{
170+
&vimtypes.VirtualDeviceConfigSpec{
171+
Operation: vimtypes.VirtualDeviceConfigSpecOperationAdd,
172+
Device: &vimtypes.VirtualDisk{
173+
VirtualDevice: vimtypes.VirtualDevice{
174+
Backing: &vimtypes.VirtualDiskFlatVer2BackingInfo{
175+
VirtualDeviceFileBackingInfo: vimtypes.VirtualDeviceFileBackingInfo{
176+
FileName: fmt.Sprintf("[%s] %s/disk-0.vmdk", ctx.Datastore.Name(), vmDirName),
177+
},
178+
},
179+
},
180+
CapacityInKB: 1024,
181+
},
182+
},
183+
&vimtypes.VirtualDeviceConfigSpec{
184+
Operation: vimtypes.VirtualDeviceConfigSpecOperationAdd,
185+
Device: &vimtypes.VirtualDisk{
186+
VirtualDevice: vimtypes.VirtualDevice{
187+
Backing: &vimtypes.VirtualDiskFlatVer2BackingInfo{
188+
VirtualDeviceFileBackingInfo: vimtypes.VirtualDeviceFileBackingInfo{
189+
FileName: fmt.Sprintf("[%s] %s/disk-1.vmdk", ctx.Datastore.Name(), vmDirName),
190+
},
191+
},
192+
},
193+
CapacityInKB: 1024,
194+
},
195+
},
196+
},
197+
}
198+
199+
createArgs := &vmlifecycle.CreateArgs{
200+
ConfigSpec: configSpec,
201+
UseContentLibrary: true,
202+
Datastores: []vmlifecycle.DatastoreRef{
203+
{
204+
MoRef: ctx.Datastore.Reference(),
205+
TopLevelDirectoryCreateSupported: true, // TLD-supported
206+
},
207+
},
208+
DatacenterMoID: ctx.Datacenter.Reference().Value,
209+
DiskPaths: []string{ctx.ContentLibraryItemDiskPath}, // Only 1 disk
210+
FilePaths: []string{ctx.ContentLibraryItemNVRAMPath},
211+
ProviderItemID: ctx.ContentLibraryItemID,
212+
}
213+
214+
// Attempt creation - should fail with "invalid disk count"
215+
_, err = vmlifecycle.CreateVirtualMachine(
216+
vmCtx,
217+
ctx.Client,
218+
ctx.RestClient,
219+
ctx.VCClient.Client,
220+
ctx.Finder,
221+
createArgs)
222+
Expect(err).To(HaveOccurred())
223+
Expect(err.Error()).To(ContainSubstring("invalid disk count"))
224+
225+
// Verify directory was cleaned up
226+
// DatastoreFileExists returns nil when file exists, os.ErrNotExist when it doesn't
227+
err = pkgutil.DatastoreFileExists(ctx, ctx.VCClient.Client, vmDirPath, ctx.Datacenter)
228+
Expect(errors.Is(err, os.ErrNotExist)).To(BeTrue(), "Directory should be cleaned up after error (should not exist)")
229+
})
230+
})
231+
})
232+
})

0 commit comments

Comments
 (0)