Delete vmdk first before delete directory for vc 6.5 or newer by emlin · Pull Request #2652 · vmware/vic

emlin · 2016-10-11T16:12:40Z

Fixes #2014
In VC 6.5, if we delete datastore file directories, with linked vmdk files inside, delete will fail, even we don't have any delta vmdks rely on them.
Delete from UI does not work as well.

The only workaround for this issue, is to delete in vSAN model, delete vmdk files first, and then file directory.

fdawg4l · 2016-10-11T16:55:34Z

lib/install/management/store_files.go


 	m := object.NewFileManager(ds.Client())
-	if d.isVSAN(ds) {
+	if d.geVC65() || d.isVSAN(ds) {


Can we do this (call deleteFiles...) all the time? Do we need to only do it for 65 or vsan?

Actually, this will make the delete slower, especially while there are many images downloaded.

hickeng · 2016-10-11T16:57:32Z

lib/install/management/store_files.go


 	m := object.NewFileManager(ds.Client())
-	if d.isVSAN(ds) {
+	if d.geVC65() || d.isVSAN(ds) {


Why is the caller responsible for knowing the exact model by which you have to delete files?

Why even bother with separate paths (this is purely an optimization)

Can we have d.deleteFiles(m, ds, dsPath) - if we really have to have the optimization, the check should be inside that function.

Here, the top level method to delete should be method
func (d *Dispatcher) deleteDatastoreFiles(ds *object.Datastore, path string, force bool) (bool, error)
So caller doesn't need to care about the internal impl, I think.

The last comment is same to @fdawg4l mentioned above. But I think delete interactively is not an optimization. It will slow down the delete.
But if vsphere is going this way, and we don't care too much about 6.0, it's also fine to remove this branch.

I don't care too much about moving to a consolidated branch, but having this logic here instead of in the pkg/vsphere/datastore code means that every single caller who wants to delete a tree has to do this same version detection or know the specifics of how to do a recursive deletion.

the pkg/vsphere/datastore is meant to be a common pkg could be used by both vic-machine and storage portlayer, but there is some assumption not exists in vic-machine.
I could refactor code to merge them two, but probably not at this time.

caglar10ur · 2016-10-11T20:33:58Z

lib/install/management/store_files.go

+func (d *Dispatcher) geVC65() bool {
+	vcVersion := d.session.Client.ServiceContent.About.Version
+	// vsphere only has versions 5.5.x, 6.0.x, 6.5.x, not likely to have 6.10.x, so compare string directly here
+	return strings.Compare(vcVersion, "6.5.0") >= 0


I think https://golang.org/pkg/strings/#HasPrefix could be more suitable here

so you mean anything without "6.0." prefix? In case, vsphere does not provide any patch like 6.1.x.
anyway, not likely...

The only reason for this method is to select a difference in a specific behaviour. If this is being broken out into a separate method then it should be something like deleteRequiresEmptyDirectory() that actually describes the use. Otherwise we'll end up with geVC70 as well once 6.5 is released and we start testing against vmkernel main.

Not quite sure about this point.
Current method name just shows the function itself, and the caller decides how to use it. That is same to the definition of isVSAN() method.

For the method in the future of geVC70, that must be other function difference between VC 6.5 and VC 7.0.
We could limit such kind of difference, and I don't prefer to have this detection, just if without this branch, there will have side effect, and that's not necessary for VC 6.0 as I said above.
So if we agree for this impact, I can remove the detection, and keep them consistent in VMFS file system and vSAN, for both VC versions.

hickeng · 2016-10-12T02:31:51Z

lib/install/management/store_files.go


 	m := object.NewFileManager(ds.Client())
-	if d.isVSAN(ds) {
+	if d.geVC65() || d.isVSAN(ds) {


I don't care too much about moving to a consolidated branch, but having this logic here instead of in the pkg/vsphere/datastore code means that every single caller who wants to delete a tree has to do this same version detection or know the specifics of how to do a recursive deletion.

hickeng · 2016-10-12T02:34:50Z

lib/install/management/store_files.go

+func (d *Dispatcher) geVC65() bool {
+	vcVersion := d.session.Client.ServiceContent.About.Version
+	// vsphere only has versions 5.5.x, 6.0.x, 6.5.x, not likely to have 6.10.x, so compare string directly here
+	return strings.Compare(vcVersion, "6.5.0") >= 0


The only reason for this method is to select a difference in a specific behaviour. If this is being broken out into a separate method then it should be something like deleteRequiresEmptyDirectory() that actually describes the use. Otherwise we'll end up with geVC70 as well once 6.5 is released and we start testing against vmkernel main.

dougm · 2016-10-12T05:26:41Z

What is the error with 6.5? And vSAN?

"Folder deletes are always recursive" http://pubs.vmware.com/vsphere-60/index.jsp#com.vmware.wssdk.apiref.doc/vim.FileManager.html#deleteFile

emlin · 2016-10-12T15:04:25Z

@dougm Well the problem in vSAN and 6.5 is slightly different.

vSAN: to delete a folder, the folder must be empty.
But it is a semi-random issue. Delete folder usually works well, but if there is any error happens, clear all files, and then delete folder will be the only solution.

6.5: delete folder works well, if there is no linked disk chain.
In our case, image pull will create vmdk files, as well as metadata file. If clear the vmdk files, delete folder works well, even with metadata files and folders exists.
So in this case, clear all files first, and then delete folder can be the workaround.

For the recursive delete, thanks for the reference. I didn't know that.
But even that's the case, if we have thousands of images, and even more layers, it will still be much better to call vsphere once and recursively delete inside of vc, than to call vsphere API thousands of times.

Do not differentiate vc6.0 and 6.5, vmfs and vsan datastore to reduce difference based on env.

hickeng · 2016-10-14T21:33:15Z

LGTM

andrewtchin · 2016-10-17T17:20:52Z

lgtm

mdubya66 · 2016-10-17T17:21:22Z

lgtm

emlin · 2016-10-18T21:23:15Z

♻️

emlin · 2016-10-18T21:23:44Z

https://ci.vmware.run/vmware/vic/6006 passed

vmwclabot added the cla-not-required label Oct 11, 2016

fdawg4l reviewed Oct 11, 2016

View reviewed changes

hickeng suggested changes Oct 11, 2016

View reviewed changes

caglar10ur reviewed Oct 11, 2016

View reviewed changes

hickeng suggested changes Oct 12, 2016

View reviewed changes

Delete vmdk first before delete directory for vc 6.5 or newer

43be2bb

emlin force-pushed the 2014 branch from c94fd64 to 088dc5e Compare October 13, 2016 15:03

Recursively delete datastore files

088dc5e

Do not differentiate vc6.0 and 6.5, vmfs and vsan datastore to reduce difference based on env.

hickeng approved these changes Oct 14, 2016

View reviewed changes

emlin added 4 commits October 17, 2016 12:21

Merge branch 'master' into 2014

93468b4

Merge branch 'master' into 2014

37fb4e2

Merge branch 'master' into 2014

4872b32

Merge branch 'master' into 2014

b23b91f

emlin merged commit 0be8891 into vmware:master Oct 18, 2016

emlin deleted the 2014 branch November 8, 2016 17:39

emlin mentioned this pull request Mar 5, 2018

Delete datastore files iteratively for all datastore types #7441

Merged

Conversation

emlin commented Oct 11, 2016

Uh oh!

fdawg4l Oct 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougm commented Oct 12, 2016

Uh oh!

emlin commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hickeng commented Oct 14, 2016

Uh oh!

andrewtchin commented Oct 17, 2016

Uh oh!

mdubya66 commented Oct 17, 2016

Uh oh!

emlin commented Oct 18, 2016

Uh oh!

emlin commented Oct 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

fdawg4l Oct 11, 2016 •

edited

Loading

emlin commented Oct 12, 2016 •

edited

Loading