)]}'
{"nova/virt/vmwareapi/volumeops.py":[{"author":{"_account_id":8247,"name":"Thang Pham","email":"thang.g.pham@gmail.com","username":"thang.pham"},"change_message_id":"c1c4008b848088c3c11b6929441bdb22d51fa635","unresolved":false,"context_lines":[{"line_number":468,"context_line":"                  {\u0027backing\u0027: volume_ref, \u0027rp\u0027: res_pool, \u0027ds\u0027: datastore,"},{"line_number":469,"context_line":"                   \u0027host\u0027: host})"},{"line_number":470,"context_line":"        try:"},{"line_number":471,"context_line":"            self._relocate_vmdk_volume(volume_ref, res_pool, datastore, host)"},{"line_number":472,"context_line":"        except oslo_vmw_exceptions.FileNotFoundException:"},{"line_number":473,"context_line":"            # Volume\u0027s vmdk was moved; remove the device so that we can"},{"line_number":474,"context_line":"            # relocate the volume."}],"source_content_type":"text/x-python","patch_set":5,"id":"9a80dd14_f1982a6c","line":471,"updated":"2015-03-18 17:10:28.000000000","message":"Should you check for the file existence before trying this and expecting a fail to correct the result?","commit_id":"7a76cac11a5156cc726bcd77bf754bb664784531"},{"author":{"_account_id":8247,"name":"Thang Pham","email":"thang.g.pham@gmail.com","username":"thang.pham"},"change_message_id":"56d1a2b646fb353b0b313b8fb9086236e03b588c","unresolved":false,"context_lines":[{"line_number":468,"context_line":"                  {\u0027backing\u0027: volume_ref, \u0027rp\u0027: res_pool, \u0027ds\u0027: datastore,"},{"line_number":469,"context_line":"                   \u0027host\u0027: host})"},{"line_number":470,"context_line":"        try:"},{"line_number":471,"context_line":"            self._relocate_vmdk_volume(volume_ref, res_pool, datastore, host)"},{"line_number":472,"context_line":"        except oslo_vmw_exceptions.FileNotFoundException:"},{"line_number":473,"context_line":"            # Volume\u0027s vmdk was moved; remove the device so that we can"},{"line_number":474,"context_line":"            # relocate the volume."}],"source_content_type":"text/x-python","patch_set":5,"id":"9a80dd14_99452771","line":471,"in_reply_to":"9a80dd14_bdf58552","updated":"2015-03-19 13:42:42.000000000","message":"Ok, thanks for explaining.","commit_id":"7a76cac11a5156cc726bcd77bf754bb664784531"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"fadfd5c7843c4bfad57eee476fd0f3aec67cba43","unresolved":false,"context_lines":[{"line_number":468,"context_line":"                  {\u0027backing\u0027: volume_ref, \u0027rp\u0027: res_pool, \u0027ds\u0027: datastore,"},{"line_number":469,"context_line":"                   \u0027host\u0027: host})"},{"line_number":470,"context_line":"        try:"},{"line_number":471,"context_line":"            self._relocate_vmdk_volume(volume_ref, res_pool, datastore, host)"},{"line_number":472,"context_line":"        except oslo_vmw_exceptions.FileNotFoundException:"},{"line_number":473,"context_line":"            # Volume\u0027s vmdk was moved; remove the device so that we can"},{"line_number":474,"context_line":"            # relocate the volume."}],"source_content_type":"text/x-python","patch_set":5,"id":"9a80dd14_bdf58552","line":471,"in_reply_to":"9a80dd14_f1982a6c","updated":"2015-03-19 09:11:54.000000000","message":"Good question.\n\nFileManager doesn\u0027t have an API for checking file existence. Also, this case is rare and we can always save one API call by not checking for file existence.","commit_id":"7a76cac11a5156cc726bcd77bf754bb664784531"},{"author":{"_account_id":8247,"name":"Thang Pham","email":"thang.g.pham@gmail.com","username":"thang.pham"},"change_message_id":"ed44cceb7556e087dceaf084b710a2138072da87","unresolved":false,"context_lines":[{"line_number":482,"context_line":""},{"line_number":483,"context_line":"        # Volume\u0027s backing is relocated now; detach the old vmdk if not done"},{"line_number":484,"context_line":"        # already."},{"line_number":485,"context_line":"        if not detached:"},{"line_number":486,"context_line":"            self.detach_disk_from_vm(volume_ref, instance, original_device,"},{"line_number":487,"context_line":"                                     destroy_disk\u003dTrue)"},{"line_number":488,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"9a80dd14_9c7d1570","line":485,"updated":"2015-03-19 13:46:20.000000000","message":"Can you put the detach_disk_from_vm call inside the try/except, since it is only used after _relocate_vmdk_volume.","commit_id":"7a76cac11a5156cc726bcd77bf754bb664784531"},{"author":{"_account_id":8247,"name":"Thang Pham","email":"thang.g.pham@gmail.com","username":"thang.pham"},"change_message_id":"15929d77e0c9b6bdc69a4e56053ca61e216eb6c5","unresolved":false,"context_lines":[{"line_number":482,"context_line":""},{"line_number":483,"context_line":"        # Volume\u0027s backing is relocated now; detach the old vmdk if not done"},{"line_number":484,"context_line":"        # already."},{"line_number":485,"context_line":"        if not detached:"},{"line_number":486,"context_line":"            self.detach_disk_from_vm(volume_ref, instance, original_device,"},{"line_number":487,"context_line":"                                     destroy_disk\u003dTrue)"},{"line_number":488,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"9a80dd14_e7691e26","line":485,"in_reply_to":"9a80dd14_87a782bb","updated":"2015-03-19 14:12:48.000000000","message":"It removes the need for the detached var, and if you put a note about it, it would help.  We can see what other people think.","commit_id":"7a76cac11a5156cc726bcd77bf754bb664784531"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"e07b97697ff4443f237f7691cb23df945a884cc9","unresolved":false,"context_lines":[{"line_number":482,"context_line":""},{"line_number":483,"context_line":"        # Volume\u0027s backing is relocated now; detach the old vmdk if not done"},{"line_number":484,"context_line":"        # already."},{"line_number":485,"context_line":"        if not detached:"},{"line_number":486,"context_line":"            self.detach_disk_from_vm(volume_ref, instance, original_device,"},{"line_number":487,"context_line":"                                     destroy_disk\u003dTrue)"},{"line_number":488,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"9a80dd14_87a782bb","line":485,"in_reply_to":"9a80dd14_9c7d1570","updated":"2015-03-19 14:08:09.000000000","message":"I can, but won\u0027t it make the code slightly difficult to understand? The try.. except is just meant for handling a specific failure during the relocate call. If you feel strongly about making this change, I can do it.","commit_id":"7a76cac11a5156cc726bcd77bf754bb664784531"},{"author":{"_account_id":8213,"name":"Claudiu Belu","email":"cbelu@cloudbasesolutions.com","username":"claudiub"},"change_message_id":"10e5b25cb8669451bc8b6466c1a0b3a172546885","unresolved":false,"context_lines":[{"line_number":473,"context_line":"                      \"reattempting relocate.\")"},{"line_number":474,"context_line":"            self.detach_disk_from_vm(volume_ref, instance, original_device)"},{"line_number":475,"context_line":"            detached \u003d True"},{"line_number":476,"context_line":"            self._relocate_vmdk_volume(volume_ref, res_pool, datastore, host)"},{"line_number":477,"context_line":""},{"line_number":478,"context_line":"        # Volume\u0027s backing is relocated now; detach the old vmdk if not done"},{"line_number":479,"context_line":"        # already."}],"source_content_type":"text/x-python","patch_set":6,"id":"7aaa499b_7e26d117","line":476,"updated":"2015-04-10 16:15:24.000000000","message":"You are passing the same arguments as the call in try. What changed to ensure that this will now pass? Just detaching it will make this call work?","commit_id":"f0972b8613ea14580f6842bee86f89a2f7508e19"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"9b4618c5c87f88ef50bff442d5a88ad0bbe2f6de","unresolved":false,"context_lines":[{"line_number":473,"context_line":"                      \"reattempting relocate.\")"},{"line_number":474,"context_line":"            self.detach_disk_from_vm(volume_ref, instance, original_device)"},{"line_number":475,"context_line":"            detached \u003d True"},{"line_number":476,"context_line":"            self._relocate_vmdk_volume(volume_ref, res_pool, datastore, host)"},{"line_number":477,"context_line":""},{"line_number":478,"context_line":"        # Volume\u0027s backing is relocated now; detach the old vmdk if not done"},{"line_number":479,"context_line":"        # already."}],"source_content_type":"text/x-python","patch_set":6,"id":"7aaa499b_eafd6c6f","line":476,"in_reply_to":"7aaa499b_7e26d117","updated":"2015-04-11 12:20:24.000000000","message":"Yes, because we are handling a corner case where storage vMotion moves the volume\u0027s disk to a different datastore assuming that it belongs to nova instance. Of course, FileNotFoundException can occur if vSphere admin inadvertently deletes the vmx file or other VM related files. In such a case, the second relocate will fail and we fail the detach volume operation.","commit_id":"f0972b8613ea14580f6842bee86f89a2f7508e19"},{"author":{"_account_id":8213,"name":"Claudiu Belu","email":"cbelu@cloudbasesolutions.com","username":"claudiub"},"change_message_id":"10e5b25cb8669451bc8b6466c1a0b3a172546885","unresolved":false,"context_lines":[{"line_number":479,"context_line":"        # already."},{"line_number":480,"context_line":"        if not detached:"},{"line_number":481,"context_line":"            self.detach_disk_from_vm(volume_ref, instance, original_device,"},{"line_number":482,"context_line":"                                     destroy_disk\u003dTrue)"},{"line_number":483,"context_line":""},{"line_number":484,"context_line":"        # Attach the current volume to the volume_ref"},{"line_number":485,"context_line":"        self.attach_disk_to_vm(volume_ref, instance,"}],"source_content_type":"text/x-python","patch_set":6,"id":"7aaa499b_7e5ed146","line":482,"updated":"2015-04-10 16:15:24.000000000","message":"This whole logic looks complicated and I don\u0027t really understand how it works.\n\nBut.\n\nIf you say that the Volume\u0027s vmdk was moved (L468), why not have something like:\n\n    volume_vmdk_moved \u003d not self._check_vmdk_exists(....)\n    if volume_vmdk_moved:\n        self.detach_disk_from_vm(....)\n\n        # this I don\u0027t get, if the vmdk was moved, how can you relocate the\n        # volume, having the old volume_ref?\n        self._relocate_vmdk_volume(volume_ref, res_pool, datastore, host)\n    else:\n        self._relocate_vmdk_volume(volume_ref, res_pool, datastore, host)\n        self.detach_disk_from_vm(volume_ref, instance, original_device, destroy_disk\u003dTrue)\n\nEven this seems unnecessary, really. You already know that it was moved, according to the L450, causing the try to always fail. This means the else I\u0027ve written now is totally useless / dead code.","commit_id":"f0972b8613ea14580f6842bee86f89a2f7508e19"},{"author":{"_account_id":9171,"name":"Vipin Balachandran","email":"vipin.bl@gmail.com","username":"vbala"},"change_message_id":"9b4618c5c87f88ef50bff442d5a88ad0bbe2f6de","unresolved":false,"context_lines":[{"line_number":479,"context_line":"        # already."},{"line_number":480,"context_line":"        if not detached:"},{"line_number":481,"context_line":"            self.detach_disk_from_vm(volume_ref, instance, original_device,"},{"line_number":482,"context_line":"                                     destroy_disk\u003dTrue)"},{"line_number":483,"context_line":""},{"line_number":484,"context_line":"        # Attach the current volume to the volume_ref"},{"line_number":485,"context_line":"        self.attach_disk_to_vm(volume_ref, instance,"}],"source_content_type":"text/x-python","patch_set":6,"id":"7aaa499b_8a74a8cf","line":482,"in_reply_to":"7aaa499b_7e5ed146","updated":"2015-04-11 12:20:24.000000000","message":"I think some of your confusion will be cleared if you understand the fact that volume in this case is a VM with an attached vmdk. This VM is called backing VM and is never powered on. During volume attach, we attach the volume\u0027s backing VM\u0027s vmdk to the nova instance.\n\nI understand that your logic is simple and I would like to include it. But the problem is, there is no API in FileManager or VirtualDiskManager to implement self._check_vmdk_exists(....)\n\n\u003ethis I don\u0027t get, if the vmdk was moved, how can you relocate the volume\nWe can, because we are relocating the backing VM after removing its disk device. \n\nWhen SDRS moves the volume\u0027s vmdk, we end up with a backing VM with a virtual device of type VirtualDisk which points to a non-existing device backing (vmdk file). The only way to repair is to remove the virtual device, relocate the backing VM and attach the vmdk file in the new location.\n\nI think we can add some more comments in _consolidate_vmdk_volume to make things clear. Also, I will try to implement _check_vmdk_exists using some existing methods even though it\u0027s not meant for checking file existence.","commit_id":"f0972b8613ea14580f6842bee86f89a2f7508e19"}]}