)]}'
{"nova/virt/vmwareapi/volumeops.py":[{"author":{"_account_id":7148,"name":"Zhu Zhu","email":"bjzzu.zz@gmail.com","username":"zhuzhubj"},"change_message_id":"9e9da54d9c0a740ecd8af027d4d32bbe54fb95ca","unresolved":false,"context_lines":[{"line_number":333,"context_line":"        # IDE does not support disk hotplug"},{"line_number":334,"context_line":"        if (instance.vm_state \u003d\u003d vm_states.ACTIVE and"},{"line_number":335,"context_line":"            adapter_type \u003d\u003d constants.ADAPTER_TYPE_IDE):"},{"line_number":336,"context_line":"            msg \u003d _(\u0027%s does not support disk hotplug.\u0027) % adapter_type"},{"line_number":337,"context_line":"            raise exception.Invalid(msg)"},{"line_number":338,"context_line":""},{"line_number":339,"context_line":"        # Attach the disk to virtual machine instance"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa98f980_c3a8ff79","line":336,"updated":"2014-09-22 06:25:12.000000000","message":"nit: suggest to add instance information and use \u0027disk attachment\u0027 sound better as this command was to call volume attachment?","commit_id":"d47beef726c0f3464256e3c46b69f13540a20ad1"},{"author":{"_account_id":8247,"name":"Thang Pham","email":"thang.g.pham@gmail.com","username":"thang.pham"},"change_message_id":"0d1012dbdc76ce76925dad73b897aec7ffc03d6a","unresolved":false,"context_lines":[{"line_number":333,"context_line":"        # IDE does not support disk hotplug"},{"line_number":334,"context_line":"        if (instance.vm_state \u003d\u003d vm_states.ACTIVE and"},{"line_number":335,"context_line":"            adapter_type \u003d\u003d constants.ADAPTER_TYPE_IDE):"},{"line_number":336,"context_line":"            msg \u003d _(\u0027%s does not support disk hotplug.\u0027) % adapter_type"},{"line_number":337,"context_line":"            raise exception.Invalid(msg)"},{"line_number":338,"context_line":""},{"line_number":339,"context_line":"        # Attach the disk to virtual machine instance"}],"source_content_type":"text/x-python","patch_set":2,"id":"da9df570_563786e3","line":336,"in_reply_to":"fa98f980_c3a8ff79","updated":"2014-09-22 15:45:05.000000000","message":"This is not just disk attachment.  This is attaching a disk to an active instance, which is usually called disk hotplug.  But it does not matter much, if we decide to support it for ide by adding the lsiLogic controller.  I am waiting for feedback from Dan or a nova-core on this, as mentioned below.","commit_id":"d47beef726c0f3464256e3c46b69f13540a20ad1"},{"author":{"_account_id":9172,"name":"Radoslav Gerganov","email":"rgerganov@vmware.com","username":"rgerganov"},"change_message_id":"3989617c1fd01ca78305f85757bbdc57f9cca822","unresolved":false,"context_lines":[{"line_number":516,"context_line":"         disk_type) \u003d vm_util.get_vmdk_path_and_adapter_type(hw_devices)"},{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        # IDE does not support disk hotplug"},{"line_number":519,"context_line":"        if (instance.vm_state \u003d\u003d vm_states.ACTIVE and"},{"line_number":520,"context_line":"            adapter_type \u003d\u003d constants.ADAPTER_TYPE_IDE):"},{"line_number":521,"context_line":"            msg \u003d _(\u0027%s does not support disk hotplug.\u0027) % adapter_type"},{"line_number":522,"context_line":"            raise exception.Invalid(msg)"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa98f980_139ece63","line":519,"updated":"2014-09-18 10:57:04.000000000","message":"what about adding an lsiLogic controller and attaching the volume there? if this works that would be a reasonable way to support hotplug in the case when the primary image is on IDE controller","commit_id":"d47beef726c0f3464256e3c46b69f13540a20ad1"},{"author":{"_account_id":8247,"name":"Thang Pham","email":"thang.g.pham@gmail.com","username":"thang.pham"},"change_message_id":"56e4718efe5dc2803d1c09ccd07b2ae4b90320d5","unresolved":false,"context_lines":[{"line_number":516,"context_line":"         disk_type) \u003d vm_util.get_vmdk_path_and_adapter_type(hw_devices)"},{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        # IDE does not support disk hotplug"},{"line_number":519,"context_line":"        if (instance.vm_state \u003d\u003d vm_states.ACTIVE and"},{"line_number":520,"context_line":"            adapter_type \u003d\u003d constants.ADAPTER_TYPE_IDE):"},{"line_number":521,"context_line":"            msg \u003d _(\u0027%s does not support disk hotplug.\u0027) % adapter_type"},{"line_number":522,"context_line":"            raise exception.Invalid(msg)"}],"source_content_type":"text/x-python","patch_set":2,"id":"baa201ad_710bebd5","line":519,"in_reply_to":"baa201ad_1ec70e65","updated":"2014-10-08 16:35:09.000000000","message":"Ok.  That sounds like a good compromise.  I\u0027ll work on a new patch for this.","commit_id":"d47beef726c0f3464256e3c46b69f13540a20ad1"},{"author":{"_account_id":8247,"name":"Thang Pham","email":"thang.g.pham@gmail.com","username":"thang.pham"},"change_message_id":"84ee786470832c7bf5a9cd8ecc349647bef938b7","unresolved":false,"context_lines":[{"line_number":516,"context_line":"         disk_type) \u003d vm_util.get_vmdk_path_and_adapter_type(hw_devices)"},{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        # IDE does not support disk hotplug"},{"line_number":519,"context_line":"        if (instance.vm_state \u003d\u003d vm_states.ACTIVE and"},{"line_number":520,"context_line":"            adapter_type \u003d\u003d constants.ADAPTER_TYPE_IDE):"},{"line_number":521,"context_line":"            msg \u003d _(\u0027%s does not support disk hotplug.\u0027) % adapter_type"},{"line_number":522,"context_line":"            raise exception.Invalid(msg)"}],"source_content_type":"text/x-python","patch_set":2,"id":"baa201ad_5cb973eb","line":519,"in_reply_to":"baa201ad_710bebd5","updated":"2014-10-10 02:46:39.000000000","message":"I took a deeper look at this.  The only way disk_bus is supported is if you use \"nova boot --block_device ...,bus\u003dscsi,...\" and this method is not supported by VMware today AFAIK.  This would only work on boot and not on \"nova volume-attach\".\n\nThis led me to patch #3.  We need to find out the adapter_type of the volume, similar to the disk_bus set in \"nova boot\".  Instead of making a get_dynamic_property vim call to the instance to find the adapter_type, it should be made to the volume (shadow VM).  This\nproperly indicates what adapter_type is needed by the volume.\nIf the instance does not have a controller to support the\nvolume adapter_type (e.g. lsiLogic), one will be added.","commit_id":"d47beef726c0f3464256e3c46b69f13540a20ad1"},{"author":{"_account_id":8247,"name":"Thang Pham","email":"thang.g.pham@gmail.com","username":"thang.pham"},"change_message_id":"00e25fcd3cbd973a52d8eb4b66038b296639798e","unresolved":false,"context_lines":[{"line_number":516,"context_line":"         disk_type) \u003d vm_util.get_vmdk_path_and_adapter_type(hw_devices)"},{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        # IDE does not support disk hotplug"},{"line_number":519,"context_line":"        if (instance.vm_state \u003d\u003d vm_states.ACTIVE and"},{"line_number":520,"context_line":"            adapter_type \u003d\u003d constants.ADAPTER_TYPE_IDE):"},{"line_number":521,"context_line":"            msg \u003d _(\u0027%s does not support disk hotplug.\u0027) % adapter_type"},{"line_number":522,"context_line":"            raise exception.Invalid(msg)"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa98f980_d137db4b","line":519,"in_reply_to":"fa98f980_139ece63","updated":"2014-09-18 14:14:11.000000000","message":"Please see https://review.openstack.org/#/c/65799/12/nova/virt/vmwareapi/vm_util.py. One of Dan Berrange\u0027s comment is \"If the user has specified \u0027ide\u0027 as their desired disk adapter type, then they have consciously opted out of using hotplug, and we shouldn\u0027t silently add another SCSI adapter behind their back\".  Hence, it is better to raise an exception, as suggested by Shawn.","commit_id":"d47beef726c0f3464256e3c46b69f13540a20ad1"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"0054d2f4e62f1871d1e18518efd4bc311e06f4ab","unresolved":false,"context_lines":[{"line_number":516,"context_line":"         disk_type) \u003d vm_util.get_vmdk_path_and_adapter_type(hw_devices)"},{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        # IDE does not support disk hotplug"},{"line_number":519,"context_line":"        if (instance.vm_state \u003d\u003d vm_states.ACTIVE and"},{"line_number":520,"context_line":"            adapter_type \u003d\u003d constants.ADAPTER_TYPE_IDE):"},{"line_number":521,"context_line":"            msg \u003d _(\u0027%s does not support disk hotplug.\u0027) % adapter_type"},{"line_number":522,"context_line":"            raise exception.Invalid(msg)"}],"source_content_type":"text/x-python","patch_set":2,"id":"baa201ad_1ec70e65","line":519,"in_reply_to":"fa98f980_3bdac017","updated":"2014-10-08 16:12:35.000000000","message":"The virt driver  \u0027attach_volume\u0027 method has a \u0027disk_bus\u0027 parameter. If this parameter requests a scsi disk bus then it is reasonable to then hotplug a SCSI controller at this time. That would be consistent with what libvirt does for hotplug, when the original guest was booted with an IDE primary disk.\n\nIf the attach_volume disk_bus parameter is not \u0027scsi\u0027 though we should raise an error","commit_id":"d47beef726c0f3464256e3c46b69f13540a20ad1"},{"author":{"_account_id":8247,"name":"Thang Pham","email":"thang.g.pham@gmail.com","username":"thang.pham"},"change_message_id":"2d538efc84ae0a6a52af0faa357d2b2619a4d517","unresolved":false,"context_lines":[{"line_number":516,"context_line":"         disk_type) \u003d vm_util.get_vmdk_path_and_adapter_type(hw_devices)"},{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        # IDE does not support disk hotplug"},{"line_number":519,"context_line":"        if (instance.vm_state \u003d\u003d vm_states.ACTIVE and"},{"line_number":520,"context_line":"            adapter_type \u003d\u003d constants.ADAPTER_TYPE_IDE):"},{"line_number":521,"context_line":"            msg \u003d _(\u0027%s does not support disk hotplug.\u0027) % adapter_type"},{"line_number":522,"context_line":"            raise exception.Invalid(msg)"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa98f980_3bdac017","line":519,"in_reply_to":"fa98f980_a5620d3b","updated":"2014-09-18 17:23:34.000000000","message":"I am on the fence on this one.  By adding the controller at volume attach, you are pushing the procedure from spawn to attach, but is it the same context as what Dan said in the review?\n\nYou are basically saying let the user specify ide disk adapter on instance creation, but ignore it on volume attach.  I like to get a broader opinion on this (maybe from Dan) before I attempt the approach you mentioned above.","commit_id":"d47beef726c0f3464256e3c46b69f13540a20ad1"},{"author":{"_account_id":9172,"name":"Radoslav Gerganov","email":"rgerganov@vmware.com","username":"rgerganov"},"change_message_id":"e59cdc133666dc5a02982c51f462b159567ca9d0","unresolved":false,"context_lines":[{"line_number":516,"context_line":"         disk_type) \u003d vm_util.get_vmdk_path_and_adapter_type(hw_devices)"},{"line_number":517,"context_line":""},{"line_number":518,"context_line":"        # IDE does not support disk hotplug"},{"line_number":519,"context_line":"        if (instance.vm_state \u003d\u003d vm_states.ACTIVE and"},{"line_number":520,"context_line":"            adapter_type \u003d\u003d constants.ADAPTER_TYPE_IDE):"},{"line_number":521,"context_line":"            msg \u003d _(\u0027%s does not support disk hotplug.\u0027) % adapter_type"},{"line_number":522,"context_line":"            raise exception.Invalid(msg)"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa98f980_a5620d3b","line":519,"in_reply_to":"fa98f980_d137db4b","updated":"2014-09-18 16:16:36.000000000","message":"Dan\u0027s comment is completely valid for the creation of the VM. My suggestion is to add controller when a volume is attached, not when the VM is created.","commit_id":"d47beef726c0f3464256e3c46b69f13540a20ad1"},{"author":{"_account_id":9172,"name":"Radoslav Gerganov","email":"rgerganov@vmware.com","username":"rgerganov"},"change_message_id":"ec263c9d33f87c3a9ad3693dbc9d62b662b71a89","unresolved":false,"context_lines":[{"line_number":322,"context_line":"        # adapter_type, disk_type"},{"line_number":323,"context_line":"        hw_devices \u003d self._session._call_method(vim_util,"},{"line_number":324,"context_line":"                                                \u0027get_dynamic_property\u0027,"},{"line_number":325,"context_line":"                                                volume_ref, \u0027VirtualMachine\u0027,"},{"line_number":326,"context_line":"                                                \u0027config.hardware.device\u0027)"},{"line_number":327,"context_line":"        (volume_vmdk_path, adapter_type,"},{"line_number":328,"context_line":"         disk_type) \u003d vm_util.get_vmdk_path_and_adapter_type(hw_devices)"}],"source_content_type":"text/x-python","patch_set":4,"id":"baa201ad_0a09362c","line":325,"updated":"2014-10-12 11:20:33.000000000","message":"This naming is confusing because volume_ref is actually a reference to a shadow virtual machine which keeps the volume, not a real reference to a volume. I see that we had this confusing naming before this patch, so I am fine to not fix it here but it is something that we definitely need to fix.","commit_id":"d727ecd94e7159ab33595aeb56eead7fed738a5a"},{"author":{"_account_id":8247,"name":"Thang Pham","email":"thang.g.pham@gmail.com","username":"thang.pham"},"change_message_id":"cda67fed80b0dd9f47c782e14ad6400c25bb4808","unresolved":false,"context_lines":[{"line_number":322,"context_line":"        # adapter_type, disk_type"},{"line_number":323,"context_line":"        hw_devices \u003d self._session._call_method(vim_util,"},{"line_number":324,"context_line":"                                                \u0027get_dynamic_property\u0027,"},{"line_number":325,"context_line":"                                                volume_ref, \u0027VirtualMachine\u0027,"},{"line_number":326,"context_line":"                                                \u0027config.hardware.device\u0027)"},{"line_number":327,"context_line":"        (volume_vmdk_path, adapter_type,"},{"line_number":328,"context_line":"         disk_type) \u003d vm_util.get_vmdk_path_and_adapter_type(hw_devices)"}],"source_content_type":"text/x-python","patch_set":4,"id":"baa201ad_a584780b","line":325,"in_reply_to":"baa201ad_0a09362c","updated":"2014-10-15 19:11:00.000000000","message":"I can make a separate patch for it.  But what is a better name? volume_shadow_vm_ref, volume_vm, volume_shadow?","commit_id":"d727ecd94e7159ab33595aeb56eead7fed738a5a"}]}