Use DatastoreFileManager to delete files #4043
Conversation
|
@mdubya66 we need your review to merge #4024 I'll also be updating vendor/govmomi again with this PR once vmware/govmomi#672 is merged. |
| @@ -86,7 +86,7 @@ func (vm *VirtualMachine) FolderName(ctx context.Context) (string, error) { | |||
| func (vm *VirtualMachine) DSPath(ctx context.Context) (url.URL, error) { | |||
| var mvm mo.VirtualMachine | |||
|
|
|||
| if err := vm.Properties(ctx, vm.Reference(), []string{"runtime.host", "config"}, &mvm); err != nil { | |||
| if err := vm.Properties(ctx, vm.Reference(), []string{"config.files.vmPathName"}, &mvm); err != nil { | |||
There was a problem hiding this comment.
Note that this is not a required change for this PR, but in the process notice we can reduce the data collected quite a bit here.
| if _, err := tasks.WaitForResult(d.ctx, func(ctx context.Context) (tasks.Task, error) { | ||
| return m.DeleteDatastoreFile(ctx, dsPath, d.session.Datacenter) | ||
| }); err != nil { | ||
| if err := m.Delete(d.ctx, dsPath); err != nil { |
There was a problem hiding this comment.
Is the retry on certain failures on needed here?
There was a problem hiding this comment.
We retry TaskInProgress faults elsewhere, but that isn't thrown by DeleteVirtualDisk_Task or DeleteDatastoreFile_Task: http://pubs.vmware.com/vsphere-65/index.jsp#com.vmware.wssdk.apiref.doc/vim.fault.TaskInProgress.html
I don't see any faults we should be retrying for those methods and the caller(s) of this code already handle the FileNotFound case.
There was a problem hiding this comment.
How are we handling NotAuthenticated issues from session timeout or similar? If there's any aspect style remediation performed in the tasks package beyond handling InProgress retrys then we should be using it across the board. Otherwise not.
@cgtexmex something else for vmomi gateway
| return tasks.Wait(context.TODO(), func(ctx context.Context) (tasks.Task, error) { | ||
| return d.fm.DeleteDatastoreFile(ctx, f, d.s.Datacenter) | ||
| }) | ||
| return d.ds.NewFileManager(d.s.Datacenter, true).Delete(ctx, f) // TODO: NewHelper should create the DatastoreFileManager |
There was a problem hiding this comment.
Is this TODO supposed to be done in other PR?
There was a problem hiding this comment.
Another PR, since this would be a cleanup/refactor that would simplify unrelated existing code, but not required to solve this issue.
| if _, err := tasks.WaitForResult(d.ctx, func(ctx context.Context) (tasks.Task, error) { | ||
| return m.DeleteDatastoreFile(ctx, dsPath, d.session.Datacenter) | ||
| }); err != nil { | ||
| if err := m.Delete(d.ctx, dsPath); err != nil { |
There was a problem hiding this comment.
How are we handling NotAuthenticated issues from session timeout or similar? If there's any aspect style remediation performed in the tasks package beyond handling InProgress retrys then we should be using it across the board. Otherwise not.
@cgtexmex something else for vmomi gateway
DatastoreFileManager is a new govmomi helper that combines FileManager, VirtualDiskManager and Datastore types for managing files on a datastore. For now the focus is on deleting files, and in particular, not leaking DOM objects on vSAN. Fixes vmware#3931
Use DatastoreFileManager to delete files
DatastoreFileManager is a new govmomi helper that combines
FileManager, VirtualDiskManager and Datastore types for managing
files on a datastore.
For now the focus is on deleting files, and in particular, not leaking
DOM objects on vSAN.
Fixes #3931