)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fd0b60b2bca4df1fe8288f7e235403794ff0f860","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Check if attachment_id exists before deleting it"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There is a case where the block device mapping might not be in"},{"line_number":10,"context_line":"the database if there are any computes running Ocata or before"},{"line_number":11,"context_line":"because the data is only persisted in the API layer when using"},{"line_number":12,"context_line":"the new attachment flow."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"This patch makes sure to check if there is an attachment_id"},{"line_number":15,"context_line":"to avoid raising exceptions due to it not being dynamically"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"3fa0c359_2811f604","line":12,"range":{"start_line":9,"start_character":0,"end_line":12,"end_character":24},"updated":"2018-02-21 14:17:40.000000000","message":"This is a bit confusing. The error during delete for a pre-scheduling failure is that the bdms are stored with the build request without an attachment_id if there are any Pike or older computes. Once all of the computes are Queens or newer, (and assuming Cinder is up to date), we\u0027ll create an attachment.\n\nI think in the normal \"local delete in the API\" scenario, if the BDM created in the API wasn\u0027t created with an attachment_id, when we load it out of the database the attachment_id column default value is nullable\u003dTrue so that gets set on the BDM object.","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"}],"nova/compute/api.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fd0b60b2bca4df1fe8288f7e235403794ff0f860","unresolved":false,"context_lines":[{"line_number":1344,"context_line":"                    else:"},{"line_number":1345,"context_line":"                        # NOTE(ildikov): This call is here only for backward"},{"line_number":1346,"context_line":"                        # compatibility can be removed after Ocata EOL."},{"line_number":1347,"context_line":"                        self._check_attach(context, volume, instance)"},{"line_number":1348,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":1349,"context_line":"                except (exception.CinderConnectionFailed,"},{"line_number":1350,"context_line":"                        exception.InvalidVolume):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa0c359_08c99aaa","line":1347,"updated":"2018-02-21 14:17:40.000000000","message":"We might want to set bdm.attachment_id \u003d None here.","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"},{"author":{"_account_id":27336,"name":"XiaojueGuan","email":"xiaojueguan@gmail.com","username":"XiaojueGuan"},"change_message_id":"4a09ab12ef465e2a7139b0c5caeaa5890305b949","unresolved":false,"context_lines":[{"line_number":2024,"context_line":"        for bdm in bdms:"},{"line_number":2025,"context_line":"            if bdm.is_volume:"},{"line_number":2026,"context_line":"                try:"},{"line_number":2027,"context_line":"                    if \u0027attachment_id\u0027 in bdm and bdm.attachment_id:"},{"line_number":2028,"context_line":"                        self.volume_api.attachment_delete(context,"},{"line_number":2029,"context_line":"                                                          bdm.attachment_id)"},{"line_number":2030,"context_line":"                    else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa0c359_687cde82","line":2027,"range":{"start_line":2027,"start_character":42,"end_line":2027,"end_character":45},"updated":"2018-02-21 14:09:42.000000000","message":"since the attachment is already existed and not none, why bother","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8129de323266b7400dcae2c910be7db2503bfb9e","unresolved":false,"context_lines":[{"line_number":2024,"context_line":"        for bdm in bdms:"},{"line_number":2025,"context_line":"            if bdm.is_volume:"},{"line_number":2026,"context_line":"                try:"},{"line_number":2027,"context_line":"                    if \u0027attachment_id\u0027 in bdm and bdm.attachment_id:"},{"line_number":2028,"context_line":"                        self.volume_api.attachment_delete(context,"},{"line_number":2029,"context_line":"                                                          bdm.attachment_id)"},{"line_number":2030,"context_line":"                    else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa0c359_28b816e3","line":2027,"range":{"start_line":2027,"start_character":42,"end_line":2027,"end_character":45},"in_reply_to":"3fa0c359_687cde82","updated":"2018-02-21 14:18:51.000000000","message":"The commit message is a bit confusing, but you should read the bug (or the test) if you don\u0027t understand the issue, rather than -1 the patch. It is perfectly OK to ask questions for clarity in a patch and not vote on it.","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fd0b60b2bca4df1fe8288f7e235403794ff0f860","unresolved":false,"context_lines":[{"line_number":3729,"context_line":"        \"\"\"Inject network info for the instance.\"\"\""},{"line_number":3730,"context_line":"        self.compute_rpcapi.inject_network_info(context, instance\u003dinstance)"},{"line_number":3731,"context_line":""},{"line_number":3732,"context_line":"    def _create_volume_bdm(self, context, instance, device, volume,"},{"line_number":3733,"context_line":"                           disk_bus, device_type, is_local_creation\u003dFalse,"},{"line_number":3734,"context_line":"                           tag\u003dNone):"},{"line_number":3735,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa0c359_2859f633","line":3732,"updated":"2018-02-21 14:17:40.000000000","message":"Also wonder if we should be setting attachment_id in here.","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e688dcc31499c05d4f5c99b0def744cb64a386bf","unresolved":false,"context_lines":[{"line_number":3729,"context_line":"        \"\"\"Inject network info for the instance.\"\"\""},{"line_number":3730,"context_line":"        self.compute_rpcapi.inject_network_info(context, instance\u003dinstance)"},{"line_number":3731,"context_line":""},{"line_number":3732,"context_line":"    def _create_volume_bdm(self, context, instance, device, volume,"},{"line_number":3733,"context_line":"                           disk_bus, device_type, is_local_creation\u003dFalse,"},{"line_number":3734,"context_line":"                           tag\u003dNone):"},{"line_number":3735,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa0c359_2886d614","line":3732,"in_reply_to":"3fa0c359_2859f633","updated":"2018-02-21 14:20:30.000000000","message":"If we did it in _check_attach_and_reserve_volume then I think we\u0027d be covered.","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"47d5eef08549a90d9ce07a5647f8f35f27218ab0","unresolved":false,"context_lines":[{"line_number":3729,"context_line":"        \"\"\"Inject network info for the instance.\"\"\""},{"line_number":3730,"context_line":"        self.compute_rpcapi.inject_network_info(context, instance\u003dinstance)"},{"line_number":3731,"context_line":""},{"line_number":3732,"context_line":"    def _create_volume_bdm(self, context, instance, device, volume,"},{"line_number":3733,"context_line":"                           disk_bus, device_type, is_local_creation\u003dFalse,"},{"line_number":3734,"context_line":"                           tag\u003dNone):"},{"line_number":3735,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa0c359_5ea758be","line":3732,"in_reply_to":"3fa0c359_2886d614","updated":"2018-02-21 16:16:34.000000000","message":"I don\u0027t think it is necessary here.  This function is called by _attach_volume and _attach_volume_shelved_offloaded.  At this point, if we do try to delete the instance, it will not follow the early delete code path and it will load the BDMs from database (which will have attachment_id \u003d None)","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fd0b60b2bca4df1fe8288f7e235403794ff0f860","unresolved":false,"context_lines":[{"line_number":3833,"context_line":"        else:"},{"line_number":3834,"context_line":"            LOG.debug(\u0027The compute service version is not high enough to \u0027"},{"line_number":3835,"context_line":"                      \u0027create a new style volume attachment.\u0027)"},{"line_number":3836,"context_line":"            self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3837,"context_line":""},{"line_number":3838,"context_line":"    def _attach_volume(self, context, instance, volume, device,"},{"line_number":3839,"context_line":"                       disk_bus, device_type, tag\u003dNone,"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa0c359_a8b98615","line":3836,"updated":"2018-02-21 14:17:40.000000000","message":"We might want to set bdm.attachment_id \u003d None here; that would handle the boot from volume and volume attach flows as well.","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"47d5eef08549a90d9ce07a5647f8f35f27218ab0","unresolved":false,"context_lines":[{"line_number":3833,"context_line":"        else:"},{"line_number":3834,"context_line":"            LOG.debug(\u0027The compute service version is not high enough to \u0027"},{"line_number":3835,"context_line":"                      \u0027create a new style volume attachment.\u0027)"},{"line_number":3836,"context_line":"            self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3837,"context_line":""},{"line_number":3838,"context_line":"    def _attach_volume(self, context, instance, volume, device,"},{"line_number":3839,"context_line":"                       disk_bus, device_type, tag\u003dNone,"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa0c359_3e904c43","line":3836,"in_reply_to":"3fa0c359_a8b98615","updated":"2018-02-21 16:16:34.000000000","message":"In my patch, I added it to _validate_bdm if we don\u0027t have an attachment_id.  Any other case would involve pulling the BDMs from the database where attachment_id will be set to None.","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fd0b60b2bca4df1fe8288f7e235403794ff0f860","unresolved":false,"context_lines":[{"line_number":3898,"context_line":"                context, instance,"},{"line_number":3899,"context_line":"                instance_actions.ATTACH_VOLUME)"},{"line_number":3900,"context_line":"            attach_volume(self, context, volume_id, instance, device,"},{"line_number":3901,"context_line":"                          volume_bdm.attachment_id)"},{"line_number":3902,"context_line":"        except Exception:"},{"line_number":3903,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3904,"context_line":"                volume_bdm.destroy()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa0c359_656967f7","line":3901,"range":{"start_line":3901,"start_character":26,"end_line":3901,"end_character":50},"updated":"2018-02-21 14:17:40.000000000","message":"I wonder about this guy.","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e688dcc31499c05d4f5c99b0def744cb64a386bf","unresolved":false,"context_lines":[{"line_number":3898,"context_line":"                context, instance,"},{"line_number":3899,"context_line":"                instance_actions.ATTACH_VOLUME)"},{"line_number":3900,"context_line":"            attach_volume(self, context, volume_id, instance, device,"},{"line_number":3901,"context_line":"                          volume_bdm.attachment_id)"},{"line_number":3902,"context_line":"        except Exception:"},{"line_number":3903,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3904,"context_line":"                volume_bdm.destroy()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa0c359_e8922e57","line":3901,"range":{"start_line":3901,"start_character":26,"end_line":3901,"end_character":50},"in_reply_to":"3fa0c359_656967f7","updated":"2018-02-21 14:20:30.000000000","message":"Setting it in _check_attach_and_reserve_volume would cover this.","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"47d5eef08549a90d9ce07a5647f8f35f27218ab0","unresolved":false,"context_lines":[{"line_number":3898,"context_line":"                context, instance,"},{"line_number":3899,"context_line":"                instance_actions.ATTACH_VOLUME)"},{"line_number":3900,"context_line":"            attach_volume(self, context, volume_id, instance, device,"},{"line_number":3901,"context_line":"                          volume_bdm.attachment_id)"},{"line_number":3902,"context_line":"        except Exception:"},{"line_number":3903,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":3904,"context_line":"                volume_bdm.destroy()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3fa0c359_1e9b1066","line":3901,"range":{"start_line":3901,"start_character":26,"end_line":3901,"end_character":50},"in_reply_to":"3fa0c359_e8922e57","updated":"2018-02-21 16:16:34.000000000","message":"\"Attach an existing volume to an existing instance.\" -- so it shouldn\u0027t be an issue.","commit_id":"85bcfcd9e2628792d5ea879ad98dcd820fa253bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e7bb1680e947f82155415d3fab6832ca53c62512","unresolved":false,"context_lines":[{"line_number":1350,"context_line":"                    # NOTE(mnaser): If we end up reserving the volume, it will"},{"line_number":1351,"context_line":"                    #               not have an attachment_id which is needed"},{"line_number":1352,"context_line":"                    #               for cleanups.  This can be removed once"},{"line_number":1353,"context_line":"                    #               all calls to reserve_volume is gone."},{"line_number":1354,"context_line":"                    if \u0027attachment_id\u0027 not in bdm:"},{"line_number":1355,"context_line":"                        bdm.attachment_id \u003d None"},{"line_number":1356,"context_line":"                except (exception.CinderConnectionFailed,"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa0c359_fecd0430","line":1353,"range":{"start_line":1353,"start_character":64,"end_line":1353,"end_character":66},"updated":"2018-02-21 15:55:39.000000000","message":"are","commit_id":"1e3761326e6d7b8b8e2669c9f1666992dc47dee1"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"47d5eef08549a90d9ce07a5647f8f35f27218ab0","unresolved":false,"context_lines":[{"line_number":1350,"context_line":"                    # NOTE(mnaser): If we end up reserving the volume, it will"},{"line_number":1351,"context_line":"                    #               not have an attachment_id which is needed"},{"line_number":1352,"context_line":"                    #               for cleanups.  This can be removed once"},{"line_number":1353,"context_line":"                    #               all calls to reserve_volume is gone."},{"line_number":1354,"context_line":"                    if \u0027attachment_id\u0027 not in bdm:"},{"line_number":1355,"context_line":"                        bdm.attachment_id \u003d None"},{"line_number":1356,"context_line":"                except (exception.CinderConnectionFailed,"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa0c359_79190e78","line":1353,"range":{"start_line":1353,"start_character":64,"end_line":1353,"end_character":66},"in_reply_to":"3fa0c359_fecd0430","updated":"2018-02-21 16:16:34.000000000","message":"I can only write in one language at a time, Python or English :)","commit_id":"1e3761326e6d7b8b8e2669c9f1666992dc47dee1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e7bb1680e947f82155415d3fab6832ca53c62512","unresolved":false,"context_lines":[{"line_number":2031,"context_line":"        for bdm in bdms:"},{"line_number":2032,"context_line":"            if bdm.is_volume:"},{"line_number":2033,"context_line":"                try:"},{"line_number":2034,"context_line":"                    if bdm.attachment_id:"},{"line_number":2035,"context_line":"                        self.volume_api.attachment_delete(context,"},{"line_number":2036,"context_line":"                                                          bdm.attachment_id)"},{"line_number":2037,"context_line":"                    else:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa0c359_1e3d50dc","line":2034,"updated":"2018-02-21 15:55:39.000000000","message":"Thinking about if we need to still handle it here. With your fix, anything new would have the attachment_id set.\n\nWith any build requests created before your fix, they might not have the bdm.attachment_id, however, the only way to get here via a build request is to delete the instance before it fails scheduling and it\u0027s created in cell0, which is likely not going to happen *before* this fix is applied, so you\u0027d already be in trouble here.\n\nSo really the check for attachment_id in the bdm should have gone into this patch which introduces the problem that this patch is fixing:\n\nhttps://review.openstack.org/#/c/545132/\n\n(which you actually pointed out yesterday)\n\nI\u0027m not sure if there is any value in adding the check here also if the attachment_id is in the bdm for existing records - probably not because realistically someone picking up https://review.openstack.org/#/c/545132/ is also going to pick up this patch in the same release (when we pull these back to stable and release them).","commit_id":"1e3761326e6d7b8b8e2669c9f1666992dc47dee1"},{"author":{"_account_id":1004,"name":"Mohammed Naser","email":"mnaser@vexxhost.com","username":"mnaser"},"change_message_id":"47d5eef08549a90d9ce07a5647f8f35f27218ab0","unresolved":false,"context_lines":[{"line_number":2031,"context_line":"        for bdm in bdms:"},{"line_number":2032,"context_line":"            if bdm.is_volume:"},{"line_number":2033,"context_line":"                try:"},{"line_number":2034,"context_line":"                    if bdm.attachment_id:"},{"line_number":2035,"context_line":"                        self.volume_api.attachment_delete(context,"},{"line_number":2036,"context_line":"                                                          bdm.attachment_id)"},{"line_number":2037,"context_line":"                    else:"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa0c359_79326ef4","line":2034,"in_reply_to":"3fa0c359_1e3d50dc","updated":"2018-02-21 16:16:34.000000000","message":"Yeah, I thought about it.  The only way this would affect you is if you had that patch only *and* deleted an instance before it was scheduled which is *very* unlikely, I agree.","commit_id":"1e3761326e6d7b8b8e2669c9f1666992dc47dee1"}]}