Skip to content

Delete datastore files iteratively for all datastore types #7441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AngieCris merged 12 commits into from
Mar 14, 2018
Merged

Delete datastore files iteratively for all datastore types #7441

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a5ff572
Revert "Reduce datastore searches during non-vSAN delete operations (…
AngieCris Mar 2, 2018
b4f1259
attempt delete datastore folder first; empty it upon failure
AngieCris Mar 8, 2018
15a37bb
fixup! attempt delete datastore folder first; empty it upon failure
AngieCris Mar 8, 2018
d7f3d62
Merge branch 'master' of github.com:vmware/vic into vic/7359
AngieCris Mar 8, 2018
3c72908
Merge branch 'master' of github.com:vmware/vic into vic/7359
AngieCris Mar 9, 2018
8045b1a
fixup! fixup! attempt delete datastore folder first; empty it upon fa…
AngieCris Mar 9, 2018
607b60f
fixup! fixup! attempt delete datastore folder first; empty it upon fa…
AngieCris Mar 9, 2018
12b4496
Merge branch 'master' of github.com:vmware/vic into vic/7359
AngieCris Mar 9, 2018
a29cd01
trigger drone
AngieCris Mar 9, 2018
25363a0
Merge branch 'master' of github.com:vmware/vic into vic/7359
AngieCris Mar 13, 2018
3ab246e
Merge branch 'master' of github.com:vmware/vic into vic/7359
AngieCris Mar 13, 2018
0b2cedc
Merge branch 'master' of github.com:vmware/vic into vic/7359
AngieCris Mar 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions lib/install/management/store_files.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,10 @@ func (d *Dispatcher) isVSAN(ds *object.Datastore) bool {
func (d *Dispatcher) deleteFilesIteratively(m *object.DatastoreFileManager, ds *object.Datastore, dsPath string) error {
defer trace.End(trace.Begin(dsPath, d.op))

Copy link
Contributor

@hickeng hickeng Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the whiplash Angie - given this isn't a direct revert if addressing anchal's comments, could we add a recursive delete attempt as the first thing, then fail over into the current path.
So adding the following at the top:

// don't check errors given detail process of any remaining files will occur after
d.deleteVMFSFiles(m, ds, dsPath)

Copy link
Contributor Author

@AngieCris AngieCris Mar 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's alright. unnecessary recursive delete doesn't sound very neat tbh....
it seems like ESX and older versions of VC don't have the issue but vSAN and new VC version do have that, in most cases it has to go thru iterative delete

if we add d.deleteVMFSFiles(m, ds, dsPath) at the top, then it would probably print failure log messages complaining not being able to delete non-empty datastore folders. Since we ignore the error coming out of this, and recursively clean stuff up later, if the folder did get cleaned up in the end, looking at the log it would still feel like that there's a failure deleting the folder.
sorry about the messy description, but just wondering will it confuse users?

Copy link
Contributor Author

@AngieCris AngieCris Mar 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log looks like this from a test run I did:

INFO[0003] Removing image stores 
DEBU[0004] Failed to delete "[datastore1] vch-test-4/VIC": Cannot delete file [datastore1] vch-test-4/VIC 
DEBU[0013] Folder "[datastore1] vch-test-4/kvStores" is not found 
DEBU[0013] Image store parent directory not empty, leaving in place.
......
DEBU[0016] Folder "[datastore1] vch-test-4" is not found 
INFO[0016] Removing Resource Pool "vch-test-4"

The later Folder "[datastore1] vch-test-4" is not found is from vm.Unregsiter, it shows that the folder is already been deleted
but still we see Failed to delete "[datastore1] vch-test-4/VIC": Cannot delete file [datastore1] vch-test-4/VIC on the top, even tho it's actually deleted successfully right after

(also we're sort of not supposed to see Folder "[datastore1] vch-test-4" is not found from unregister at all, but it shows up because the line we added returns NotFound error)

Copy link
Contributor

@hickeng hickeng Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with them showing up as debug messages - we could add an additional debug message saying "Attempting recursive delete" then if that's not clean a message saying "Cleaning up recusive deletion failures via iterative delete" or similar.

We may want to return a different error type rather than re-use NotFound if it's causing misleading output.

Copy link
Contributor Author

@AngieCris AngieCris Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Here's the log output with the most recent change:

DEBU[0007] No disks found attached to VM                
INFO[0007] Removing image stores                        
DEBU[0009] Failed to delete "[datastore1] vch-2/VIC": Cannot delete file [datastore1] vch-2/VIC 
DEBU[0009] Attempt to delete top level folder "[datastore1] vch-2/VIC" failed. Remove the children files instead. 
DEBU[0027] Image store parent directory not empty, leaving in place. 
INFO[0027] Removing volume stores

Does this look okay? I added a log saying Attempt to delete top level folder failed. Remove the children files instead

if d.isVSAN(ds) {
// Get sorted result to make sure child files are listed ahead of their parent folder so we empty the folder before deleting it.
// This behaviour is specifically for vSan, as vSan sometimes throws an error when deleting a folder that is not empty.
// If deleting top level folder fails, remove the child files to empty the folder first
err := d.deleteVMFSFiles(m, ds, dsPath)
if err != nil {
d.op.Debugf("Attempt to delete top level folder %s failed. Remove the children files instead.", dsPath)
res, err := d.getSortedChildren(ds, dsPath)
if err != nil {
if !types.IsFileNotFound(err) {
Expand All @@ -176,9 +177,11 @@ func (d *Dispatcher) deleteFilesIteratively(m *object.DatastoreFileManager, ds *
return err
}
}

return d.deleteVMFSFiles(m, ds, dsPath)
}

return d.deleteVMFSFiles(m, ds, dsPath)
return nil
}

func (d *Dispatcher) deleteVMFSFiles(m *object.DatastoreFileManager, ds *object.Datastore, dsPath string) error {
Expand All @@ -193,7 +196,9 @@ func (d *Dispatcher) deleteVMFSFiles(m *object.DatastoreFileManager, ds *object.

if err := m.Delete(d.op, dsPath); err != nil {
d.op.Debugf("Failed to delete %q: %s", dsPath, err)
return err
}

return nil
}

Expand Down