cloudfoundry / bosh-vsphere-cpi-release

BOSH vSphere CPI
Apache License 2.0
31 stars 35 forks source link

Moving persistent disk on VM delete breaks if datastore has a space in the name #38

Closed amhuber closed 6 years ago

amhuber commented 6 years ago

Before a VM with a persistent disk is deleted, the CPI checks the vAppConfig property to determine the original path of the persistent VMDK and will move it back in cases where the VM has been migrated to a new datastore and the disk has been moved into the VM folder. We've been trying to troubleshoot why the move function fails in some cases with this error:

CPI 'delete_vm' method responded with error: CmdError{"type":"Unknown","message":"Invalid datastore path '[some-datastore Folder'.","ok_to_retry":false}

Looking at the code at https://github.com/cloudfoundry-incubator/bosh-vsphere-cpi-release/blob/master/src/vsphere_cpi/lib/cloud/vsphere/resources/vm.rb#L297-L299, you are splitting the path on a space character which breaks if the datastore name contains a space. For example, this will work fine:

[some-datastore-path] Folder/disk-guid.vmdk

This will break:

[some datastore path] Folder/disk-guid.vmdk

In other places in the code you're using a regular expression to grab the text between the brackets to determine the name, splitting on a space is not safe.

cdutra commented 6 years ago

@amhuber Thanks for filling this issue. Indeed a regular expressions would be better. I added a story for that: https://www.pivotaltracker.com/story/show/151539797

ktchen14 commented 6 years ago

This should be fixed by d17f5c46cfa56655826422c217e9e038a0c053da.

amhuber commented 6 years ago

Awesome, thank you. Looks like that will fix our issue.

EleanorRigby commented 6 years ago

@yeshwantbabar @ktchen14 : Can we close this issue now?

amhuber commented 6 years ago

Yes, we've confirmed that the new release is working correctly. Thank you, this saves us a huge amount of pain not having to manually fix the disks on every deployment. :-)

EleanorRigby commented 6 years ago

@amhuber : Thanks for reporting the issue and the update for closing it.

scottillogical commented 6 years ago

Ran into this, looking to upgrade to resolve but curious @amhuber what process did you do to manually fix the disks?

amhuber commented 6 years ago

Assuming you have a VM that is already broken, then the CPI would have deleted the VM in question but left the disk in the VM folder. To fix it, you need to:

scottillogical commented 6 years ago

Thanks for the update @amhuber