)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"68cb48eb4fa40ce29d9131d62d4b154ac5befd43","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Call volume detach even if terminate_connection fails"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When an instance is booted from volume while"},{"line_number":10,"context_line":"specifying yes for \"Delete Volume on Instance"},{"line_number":11,"context_line":"delete\", then if at instance deletion it happens"},{"line_number":12,"context_line":"to take too long for cinder volume driver terminate"},{"line_number":13,"context_line":"connection to return, then the volume is not"},{"line_number":14,"context_line":"removed."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"This fix improves error handling for this specific"},{"line_number":17,"context_line":"case, and calls detach even if volume"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"7faddb67_dd525d3b","line":14,"range":{"start_line":9,"start_character":0,"end_line":14,"end_character":8},"updated":"2019-07-16 13:18:26.000000000","message":"Your actual change is pretty generic as it\u0027s touching code in _shutdown_instance that\u0027s obviously used outside of this limited instance deletion use case. You might want to reword this.","commit_id":"37821894a4f3eb18432299dc1378a9187a6920f4"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"19275220b055c5ec14fd2ec75f89b6b0f75157f9","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Francois Palin \u003cfpalin@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2019-11-13 16:40:19 -0500"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Add retry to cinder api calls related to volume detach"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When shutting down an instance for which volume needs to be"},{"line_number":10,"context_line":"deleted, if it happens to take too long for cinder volume"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"3fa7e38b_486edf64","line":7,"range":{"start_line":7,"start_character":20,"end_line":7,"end_character":23},"updated":"2019-12-05 15:07:39.000000000","message":"API","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7ad65b1b9b3fc90bf0c9589a1995b16feca93a20","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Add retry to cinder api calls related to volume detach"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When shutting down an instance for which volume needs to be"},{"line_number":10,"context_line":"deleted, if it happens to take too long for cinder volume"},{"line_number":11,"context_line":"driver terminate connection to return, then an unknown"},{"line_number":12,"context_line":"cinder exception is received and the volume is not removed."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"This fix adds a retry mechanism directly in cinder api calls"},{"line_number":15,"context_line":"attachment_delete, terminate_connection, and detach."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"3fa7e38b_28f7e35c","line":12,"range":{"start_line":10,"start_character":9,"end_line":12,"end_character":32},"updated":"2019-12-05 15:10:59.000000000","message":"You mean a MessagingTimeout from the cinder API, right? Because the API call in cinder is a blocking RPC call and the backend times out the default rpc_response_timeout of 60 seconds and then we orphan the volume.\n\nHow does retrying fix that? I guess because the original request is being processed and eventually completes, so a subsequent request comes into cinder and cinder says it\u0027s already done and there is nothing to do. Are there cinder backends that would fail though if you try to terminate a connection or delete an attachment that is already terminated/deleted?","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"93bce0043d5699fa0e2187295180e733d0259531","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Add retry to cinder api calls related to volume detach"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When shutting down an instance for which volume needs to be"},{"line_number":10,"context_line":"deleted, if it happens to take too long for cinder volume"},{"line_number":11,"context_line":"driver terminate connection to return, then an unknown"},{"line_number":12,"context_line":"cinder exception is received and the volume is not removed."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"This fix adds a retry mechanism directly in cinder api calls"},{"line_number":15,"context_line":"attachment_delete, terminate_connection, and detach."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"3fa7e38b_b206bc84","line":12,"range":{"start_line":10,"start_character":9,"end_line":12,"end_character":32},"in_reply_to":"3fa7e38b_28f7e35c","updated":"2020-01-23 00:16:16.000000000","message":"\u003e Are there cinder backends that would fail though if you try to terminate\n \u003e a connection or delete an attachment that is already terminated/deleted?\n\nThe drivers are supposed to be able to handle this, though it\u0027s difficult to say whether they all do.  The reference drivers check to make sure something exists before trying to delete from the backend, so hopefully the drivers for backends where this might be a problem took the hint to do likewise.","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"}],"nova/compute/manager.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"0223dbebe086dc8691c95950de4c5f7b5e14ec28","unresolved":false,"context_lines":[{"line_number":2623,"context_line":"                                                             bdm.volume_id,"},{"line_number":2624,"context_line":"                                                             connector)"},{"line_number":2625,"context_line":"                    # allow calling detach even if terminate_connection fails"},{"line_number":2626,"context_line":"                    except cinder_exception.ClientException as exc:"},{"line_number":2627,"context_line":"                        LOG.warning(\u0027Ignoring unknown cinder exception for \u0027"},{"line_number":2628,"context_line":"                                    \u0027volume %(volume_id)s: %(exc)s ,\u0027"},{"line_number":2629,"context_line":"                                    \u0027following terminate connection\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_e468fdb9","line":2626,"updated":"2019-07-14 12:49:11.000000000","message":"Why not retry on this? Note attachment_delete above is the new style for this and could fail the same way.","commit_id":"37821894a4f3eb18432299dc1378a9187a6920f4"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"68cb48eb4fa40ce29d9131d62d4b154ac5befd43","unresolved":false,"context_lines":[{"line_number":2623,"context_line":"                                                             bdm.volume_id,"},{"line_number":2624,"context_line":"                                                             connector)"},{"line_number":2625,"context_line":"                    # allow calling detach even if terminate_connection fails"},{"line_number":2626,"context_line":"                    except cinder_exception.ClientException as exc:"},{"line_number":2627,"context_line":"                        LOG.warning(\u0027Ignoring unknown cinder exception for \u0027"},{"line_number":2628,"context_line":"                                    \u0027volume %(volume_id)s: %(exc)s ,\u0027"},{"line_number":2629,"context_line":"                                    \u0027following terminate connection\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_9dd385d5","line":2626,"in_reply_to":"7faddb67_e468fdb9","updated":"2019-07-16 13:18:26.000000000","message":"Yup agreed, we need to handle both and could potentially retry. \n\nThat said the associated bug and commit message describe a timeout between c-api and c-vol causing this so I\u0027m not sure how useful a retry would actually be.\n\nCould we also have cinderclient raise something more specific here to highlight this is a timeout between c-api and c-vol rather than a generic error? I appreciate that we only get a 500 back from c-api but doesn\u0027t that contain something we could use to raise an more meaningful exception?","commit_id":"37821894a4f3eb18432299dc1378a9187a6920f4"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"77fcbd1ed38b9549b0772a3ec542ec7229a5ec52","unresolved":false,"context_lines":[{"line_number":2631,"context_line":"                                    instance\u003dinstance)"},{"line_number":2632,"context_line":""},{"line_number":2633,"context_line":"                    self.volume_api.detach(context, bdm.volume_id,"},{"line_number":2634,"context_line":"                                           instance.uuid)"},{"line_number":2635,"context_line":""},{"line_number":2636,"context_line":"            except exception.VolumeAttachmentNotFound as exc:"},{"line_number":2637,"context_line":"                LOG.debug(\u0027Ignoring VolumeAttachmentNotFound: %s\u0027, exc,"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_2d1cc376","line":2634,"updated":"2019-07-12 22:17:48.000000000","message":"I think this is something we do not want to do. This will flip the cinder volume status to \"detached\" when the volume was not actually detached from the cinder storage backend. Which will result in the volume being made available for attachment by others when it is still attached to the original instance.\n\nI\u0027m adding lyarwood to this review to sanity check me.","commit_id":"37821894a4f3eb18432299dc1378a9187a6920f4"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"68cb48eb4fa40ce29d9131d62d4b154ac5befd43","unresolved":false,"context_lines":[{"line_number":2631,"context_line":"                                    instance\u003dinstance)"},{"line_number":2632,"context_line":""},{"line_number":2633,"context_line":"                    self.volume_api.detach(context, bdm.volume_id,"},{"line_number":2634,"context_line":"                                           instance.uuid)"},{"line_number":2635,"context_line":""},{"line_number":2636,"context_line":"            except exception.VolumeAttachmentNotFound as exc:"},{"line_number":2637,"context_line":"                LOG.debug(\u0027Ignoring VolumeAttachmentNotFound: %s\u0027, exc,"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_ddc49df7","line":2634,"in_reply_to":"7faddb67_2d1cc376","updated":"2019-07-16 13:18:26.000000000","message":"terminate_connection only unmaps the volume from a specific compute host so I\u0027d actually be okay with this in that context. It would allow the volume to be reused and allow operators to manually clean up the mapping.\n\nI need to go over attachment_delete but I think there\u0027s additional logic in there so without a specific timeout exception being raised I don\u0027t think it would be safe to just swallow a failure there and detach the volume.","commit_id":"37821894a4f3eb18432299dc1378a9187a6920f4"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"ec57998923410fc36c6abc945f76e81ff91c69bb","unresolved":false,"context_lines":[{"line_number":2610,"context_line":"        timer.restart()"},{"line_number":2611,"context_line":"        for bdm in vol_bdms:"},{"line_number":2612,"context_line":"            try:"},{"line_number":2613,"context_line":"                attempts \u003d 2"},{"line_number":2614,"context_line":"                for attempt in range(1, attempts + 1):"},{"line_number":2615,"context_line":"                    try:"},{"line_number":2616,"context_line":"                        if bdm.attachment_id:"}],"source_content_type":"text/x-python","patch_set":3,"id":"7faddb67_6011d55d","line":2613,"range":{"start_line":2613,"start_character":27,"end_line":2613,"end_character":28},"updated":"2019-08-08 14:31:30.000000000","message":"Should we make this configurable?\n\nMaybe also increase the default to 3 or 5?","commit_id":"4475affcdfb52e432a3f712da5fea7175566f350"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5c788ab7d12f456f4d2f5d0962bd1bcdeb7d8f5d","unresolved":false,"context_lines":[{"line_number":2610,"context_line":"        timer.restart()"},{"line_number":2611,"context_line":"        for bdm in vol_bdms:"},{"line_number":2612,"context_line":"            try:"},{"line_number":2613,"context_line":"                attempts \u003d 2"},{"line_number":2614,"context_line":"                for attempt in range(1, attempts + 1):"},{"line_number":2615,"context_line":"                    try:"},{"line_number":2616,"context_line":"                        if bdm.attachment_id:"}],"source_content_type":"text/x-python","patch_set":3,"id":"7faddb67_f186f365","line":2613,"range":{"start_line":2613,"start_character":27,"end_line":2613,"end_character":28},"in_reply_to":"7faddb67_6011d55d","updated":"2019-08-08 20:53:48.000000000","message":"I\u0027d like to avoid new configs for this, but you could potentially piggy back on these:\n\nhttps://docs.openstack.org/nova/latest/configuration/config.html#hyperv.volume_attach_retry_count\n\nhttps://docs.openstack.org/nova/latest/configuration/config.html#hyperv.volume_attach_retry_interval","commit_id":"4475affcdfb52e432a3f712da5fea7175566f350"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"618137570eed4926fbeb2cb4df20dbe807899ffe","unresolved":false,"context_lines":[{"line_number":2610,"context_line":"        timer.restart()"},{"line_number":2611,"context_line":"        for bdm in vol_bdms:"},{"line_number":2612,"context_line":"            try:"},{"line_number":2613,"context_line":"                attempts \u003d 2"},{"line_number":2614,"context_line":"                for attempt in range(1, attempts + 1):"},{"line_number":2615,"context_line":"                    try:"},{"line_number":2616,"context_line":"                        if bdm.attachment_id:"}],"source_content_type":"text/x-python","patch_set":3,"id":"7faddb67_f5b530d1","line":2613,"range":{"start_line":2613,"start_character":27,"end_line":2613,"end_character":28},"in_reply_to":"7faddb67_f186f365","updated":"2019-08-29 13:13:17.000000000","message":"I wouldn\u0027t reuse these, they are currently used to control the retrying of calls down to os-brick and not to c-api as in this case.\n\nI\u0027d still introduce new configurables here, if anything these hyperv specific configurables could be extracted into generic configurables used across the virt drivers when making calls down to os-brick.","commit_id":"4475affcdfb52e432a3f712da5fea7175566f350"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"ec57998923410fc36c6abc945f76e81ff91c69bb","unresolved":false,"context_lines":[{"line_number":2610,"context_line":"        timer.restart()"},{"line_number":2611,"context_line":"        for bdm in vol_bdms:"},{"line_number":2612,"context_line":"            try:"},{"line_number":2613,"context_line":"                attempts \u003d 2"},{"line_number":2614,"context_line":"                for attempt in range(1, attempts + 1):"},{"line_number":2615,"context_line":"                    try:"},{"line_number":2616,"context_line":"                        if bdm.attachment_id:"},{"line_number":2617,"context_line":"                            api_call \u003d \u0027attachment delete\u0027"},{"line_number":2618,"context_line":"                            self.volume_api.attachment_delete(context,"}],"source_content_type":"text/x-python","patch_set":3,"id":"7faddb67_e07dc5b2","line":2615,"range":{"start_line":2613,"start_character":0,"end_line":2615,"end_character":24},"updated":"2019-08-08 14:31:30.000000000","message":"You could use the retrying module for this by extracting the below into a separate method with the @retrying.retry decorator. There\u0027s a few examples in the codebase already:\n\nhttp://codesearch.openstack.org/?q\u003d%40retrying.retry\u0026i\u003dnope\u0026files\u003d\u0026repos\u003dopenstack/nova","commit_id":"4475affcdfb52e432a3f712da5fea7175566f350"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"bca15d787b37d27e865bf85d368f24fa26e898d8","unresolved":false,"context_lines":[{"line_number":2610,"context_line":"        timer.restart()"},{"line_number":2611,"context_line":"        for bdm in vol_bdms:"},{"line_number":2612,"context_line":"            try:"},{"line_number":2613,"context_line":"                attempts \u003d 2"},{"line_number":2614,"context_line":"                for attempt in range(1, attempts + 1):"},{"line_number":2615,"context_line":"                    try:"},{"line_number":2616,"context_line":"                        if bdm.attachment_id:"},{"line_number":2617,"context_line":"                            api_call \u003d \u0027attachment delete\u0027"},{"line_number":2618,"context_line":"                            self.volume_api.attachment_delete(context,"}],"source_content_type":"text/x-python","patch_set":3,"id":"7faddb67_f1d4b349","line":2615,"range":{"start_line":2613,"start_character":0,"end_line":2615,"end_character":24},"in_reply_to":"7faddb67_e07dc5b2","updated":"2019-08-08 20:56:18.000000000","message":"I definitely agree with splitting this out into a separate method, this method is already getting huge and hard to unit test properly.\n\nWhat I\u0027d suggest doing is a 2-patch series, one where you refactor this delete/terminate code out - which won\u0027t have any test changes, and then another which adds the try/except + retyr logic you want and then you can unit test just that without the rest of the complexity in the _shutdown_instance method.","commit_id":"4475affcdfb52e432a3f712da5fea7175566f350"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"744f3d2c85cf4b171e2182ba5b302513a8ac6e22","unresolved":false,"context_lines":[{"line_number":2625,"context_line":"        timer.restart()"},{"line_number":2626,"context_line":"        for bdm in vol_bdms:"},{"line_number":2627,"context_line":"            try:"},{"line_number":2628,"context_line":"                connector \u003d None"},{"line_number":2629,"context_line":"                if not bdm.attachment_id:"},{"line_number":2630,"context_line":"                    connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":2631,"context_line":""},{"line_number":2632,"context_line":"                try:"},{"line_number":2633,"context_line":"                    self._volume_detach_delete(context, bdm, connector)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa7e38b_47d17057","line":2630,"range":{"start_line":2628,"start_character":0,"end_line":2630,"end_character":74},"updated":"2019-09-16 15:12:04.000000000","message":"Can we just move this into _volume_detach_delete ?","commit_id":"9f16721708097e4bc3fe8302deca56507fd965fa"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"744f3d2c85cf4b171e2182ba5b302513a8ac6e22","unresolved":false,"context_lines":[{"line_number":2638,"context_line":"                                    {\u0027exc\u0027: exc, \u0027volume_id\u0027: bdm.volume_id},"},{"line_number":2639,"context_line":"                                    instance\u003dinstance)"},{"line_number":2640,"context_line":""},{"line_number":2641,"context_line":"                if not bdm.attachment_id:"},{"line_number":2642,"context_line":"                    self.volume_api.detach(context, bdm.volume_id,"},{"line_number":2643,"context_line":"                                           instance.uuid)"},{"line_number":2644,"context_line":""},{"line_number":2645,"context_line":"            except exception.VolumeAttachmentNotFound as exc:"},{"line_number":2646,"context_line":"                LOG.debug(\u0027Ignoring VolumeAttachmentNotFound: %s\u0027, exc,"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa7e38b_6764acba","line":2643,"range":{"start_line":2641,"start_character":0,"end_line":2643,"end_character":57},"updated":"2019-09-16 15:12:04.000000000","message":"Move this into _volume_detach_delete","commit_id":"9f16721708097e4bc3fe8302deca56507fd965fa"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"655ad301fd44464087404ed55b4f014d63903362","unresolved":false,"context_lines":[{"line_number":2551,"context_line":"        if bdm.attachment_id:"},{"line_number":2552,"context_line":"            self.volume_api.attachment_delete(context, bdm.attachment_id)"},{"line_number":2553,"context_line":"        else:"},{"line_number":2554,"context_line":"            # NOTE(vish): actual driver detach done in"},{"line_number":2555,"context_line":"            #             driver.destroy, so just tell cinder"},{"line_number":2556,"context_line":"            #             that we are done with it."},{"line_number":2557,"context_line":"            connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":2558,"context_line":"            self.volume_api.terminate_connection(context,"},{"line_number":2559,"context_line":"                                                 bdm.volume_id,"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_283295a8","line":2556,"range":{"start_line":2554,"start_character":0,"end_line":2556,"end_character":51},"updated":"2019-09-23 08:46:39.000000000","message":"nit - you can wrap this onto two lines I think, it doesn\u0027t need to be indented.","commit_id":"613f2162989082f72dd281b9786ec834cc92a759"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"6a18aecbf343a89f30ef2b79e081bb8f7b37640d","unresolved":false,"context_lines":[{"line_number":2547,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d"},{"line_number":2548,"context_line":"                        CONF.compute.volume_detach_retry_count,"},{"line_number":2549,"context_line":"                    retry_on_exception\u003dlambda e: isinstance("},{"line_number":2550,"context_line":"                        e, cinder_exception.ClientException))"},{"line_number":2551,"context_line":"    def _volume_detach_delete(self, context, bdm, instance):"},{"line_number":2552,"context_line":"        if bdm.attachment_id:"},{"line_number":2553,"context_line":"            self.volume_api.attachment_delete(context, bdm.attachment_id)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3fa7e38b_db5605cf","line":2550,"range":{"start_line":2550,"start_character":27,"end_line":2550,"end_character":59},"updated":"2019-10-10 13:29:11.000000000","message":"Ack: \"The base exception class for all exceptions this library raises.\"\n\nSo we will retry on *any* cinder exception.","commit_id":"bcd3177d5055b48f9e67e236f70bfd2340dee049"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"58b46c449f923b61ffa1f1ca1c0198cf776bfb5d","unresolved":false,"context_lines":[{"line_number":2547,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d"},{"line_number":2548,"context_line":"                        CONF.compute.volume_detach_retry_count,"},{"line_number":2549,"context_line":"                    retry_on_exception\u003dlambda e: isinstance("},{"line_number":2550,"context_line":"                        e, cinder_exception.ClientException))"},{"line_number":2551,"context_line":"    def _volume_detach_delete(self, context, bdm, instance):"},{"line_number":2552,"context_line":"        if bdm.attachment_id:"},{"line_number":2553,"context_line":"            self.volume_api.attachment_delete(context, bdm.attachment_id)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3fa7e38b_5baba0d4","line":2550,"range":{"start_line":2550,"start_character":27,"end_line":2550,"end_character":59},"in_reply_to":"3fa7e38b_7a6ea42f","updated":"2019-10-16 11:45:41.000000000","message":"I think what you\u0027ve written here is correct. I was just calling out for the benefit of other reviewers that I\u0027d actually checked.\n\nDon\u0027t change this on my account!","commit_id":"bcd3177d5055b48f9e67e236f70bfd2340dee049"},{"author":{"_account_id":30009,"name":"François Palin","email":"fpalin@redhat.com","username":"FrancoisPalin"},"change_message_id":"0e5484f804b5192a085ad178d6554df3c7f013ef","unresolved":false,"context_lines":[{"line_number":2547,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d"},{"line_number":2548,"context_line":"                        CONF.compute.volume_detach_retry_count,"},{"line_number":2549,"context_line":"                    retry_on_exception\u003dlambda e: isinstance("},{"line_number":2550,"context_line":"                        e, cinder_exception.ClientException))"},{"line_number":2551,"context_line":"    def _volume_detach_delete(self, context, bdm, instance):"},{"line_number":2552,"context_line":"        if bdm.attachment_id:"},{"line_number":2553,"context_line":"            self.volume_api.attachment_delete(context, bdm.attachment_id)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3fa7e38b_7a6ea42f","line":2550,"range":{"start_line":2550,"start_character":27,"end_line":2550,"end_character":59},"in_reply_to":"3fa7e38b_db5605cf","updated":"2019-10-15 17:46:34.000000000","message":"I will make use of type instead of isinstance to make sure this is true only when base class ClientException","commit_id":"bcd3177d5055b48f9e67e236f70bfd2340dee049"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"6a18aecbf343a89f30ef2b79e081bb8f7b37640d","unresolved":false,"context_lines":[{"line_number":2555,"context_line":"            # NOTE(vish): actual driver detach done in driver.destroy,"},{"line_number":2556,"context_line":"            #             so just tell cinder that we are done with it."},{"line_number":2557,"context_line":"            connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":2558,"context_line":"            self.volume_api.terminate_connection(context,"},{"line_number":2559,"context_line":"                                                 bdm.volume_id,"},{"line_number":2560,"context_line":"                                                 connector)"},{"line_number":2561,"context_line":"            self.volume_api.detach(context, bdm.volume_id,"},{"line_number":2562,"context_line":"                                   instance.uuid)"},{"line_number":2563,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3fa7e38b_dbcda530","line":2560,"range":{"start_line":2558,"start_character":0,"end_line":2560,"end_character":59},"updated":"2019-10-10 13:29:11.000000000","message":"In the event that detach fails we will retry terminate_connection(). From a brief look at the implementation, it seems that whether this fails or not for a connection which was previously terminated is delegated to the volume backend driver. The couple I looked at didn\u0027t look like they would fail, but I still think we could be more conservative here. I think it would be more robust to retry terminate_connection until it works, then detach until it works.\n\ne.g.\n\ndef _volume_detach_delete():\n  @retrying.retry(...)\n  def _retry(f, *args, **kwargs):\n    return f(*args, **kwargs)\n\n  if bdm.attachment_id:\n    _retry(self.volume_api.attachment_delete, context, ...)\n  else:\n    connector \u003d self.driver.get_volume_connector(instance)\n    _retry(self.volume_api.terminate_connection, context, ...)\n    _retry(self.volume_api.detach, context, ...)\n\nThoughts?","commit_id":"bcd3177d5055b48f9e67e236f70bfd2340dee049"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"9f76244b4e415055256fb877395160739a481d05","unresolved":false,"context_lines":[{"line_number":2555,"context_line":"            # NOTE(vish): actual driver detach done in driver.destroy,"},{"line_number":2556,"context_line":"            #             so just tell cinder that we are done with it."},{"line_number":2557,"context_line":"            connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":2558,"context_line":"            self.volume_api.terminate_connection(context,"},{"line_number":2559,"context_line":"                                                 bdm.volume_id,"},{"line_number":2560,"context_line":"                                                 connector)"},{"line_number":2561,"context_line":"            self.volume_api.detach(context, bdm.volume_id,"},{"line_number":2562,"context_line":"                                   instance.uuid)"},{"line_number":2563,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3fa7e38b_9537a0b0","line":2560,"range":{"start_line":2558,"start_character":0,"end_line":2560,"end_character":59},"in_reply_to":"3fa7e38b_7afb0478","updated":"2019-11-04 14:21:57.000000000","message":"Apologies I missed that you were waiting on my input here.\n\nI agree that it would be more robust to retry both but if we are going to do that then I\u0027d decorate attachment_delete, terminate_connection and detach in nova/volume/cinder.py instead of private _retry methods here. IMHO it\u0027s useful in any context they are called.","commit_id":"bcd3177d5055b48f9e67e236f70bfd2340dee049"},{"author":{"_account_id":30009,"name":"François Palin","email":"fpalin@redhat.com","username":"FrancoisPalin"},"change_message_id":"0e5484f804b5192a085ad178d6554df3c7f013ef","unresolved":false,"context_lines":[{"line_number":2555,"context_line":"            # NOTE(vish): actual driver detach done in driver.destroy,"},{"line_number":2556,"context_line":"            #             so just tell cinder that we are done with it."},{"line_number":2557,"context_line":"            connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":2558,"context_line":"            self.volume_api.terminate_connection(context,"},{"line_number":2559,"context_line":"                                                 bdm.volume_id,"},{"line_number":2560,"context_line":"                                                 connector)"},{"line_number":2561,"context_line":"            self.volume_api.detach(context, bdm.volume_id,"},{"line_number":2562,"context_line":"                                   instance.uuid)"},{"line_number":2563,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3fa7e38b_7afb0478","line":2560,"range":{"start_line":2558,"start_character":0,"end_line":2560,"end_character":59},"in_reply_to":"3fa7e38b_dbcda530","updated":"2019-10-15 17:46:34.000000000","message":"Thanks for taking the time, Matt. I agree with your suggestion, this way we better focus the retry on where the issue is. I\u0027m also interested in what Lee thinks about this, before I go for a respin.","commit_id":"bcd3177d5055b48f9e67e236f70bfd2340dee049"}],"nova/conf/compute.py":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"744f3d2c85cf4b171e2182ba5b302513a8ac6e22","unresolved":false,"context_lines":[{"line_number":971,"context_line":"        default\u003d5,"},{"line_number":972,"context_line":"        min\u003d1,"},{"line_number":973,"context_line":"        help\u003d\"\"\""},{"line_number":974,"context_line":"Volume detach retry count"},{"line_number":975,"context_line":""},{"line_number":976,"context_line":"The number of times to retry detaching a volume. Volume detach"},{"line_number":977,"context_line":"is retried until success or the given retry count is reached."}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa7e38b_24eeb288","line":974,"range":{"start_line":974,"start_character":0,"end_line":974,"end_character":25},"updated":"2019-09-16 15:12:04.000000000","message":"How about `Volume API detach call retry count`?","commit_id":"9f16721708097e4bc3fe8302deca56507fd965fa"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"a0f9b62723381becd6bb9bf2da6a6138a76c0e30","unresolved":false,"context_lines":[{"line_number":862,"context_line":"        help\u003d\"\"\""},{"line_number":863,"context_line":"Volume API detach call retry count"},{"line_number":864,"context_line":""},{"line_number":865,"context_line":"The number of times to retry detaching a volume. Volume detach"},{"line_number":866,"context_line":"is retried until success or the given retry count is reached."},{"line_number":867,"context_line":""},{"line_number":868,"context_line":"Possible values:"}],"source_content_type":"text/x-python","patch_set":8,"id":"3fa7e38b_237d1db2","line":865,"range":{"start_line":865,"start_character":56,"end_line":865,"end_character":62},"updated":"2019-11-08 11:48:15.000000000","message":"detach, terminate_connection and attachment_delete.","commit_id":"2c69428553e6c2bbda56b48827814a0353417ca1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"add0f65e41df311deee3302b81c84951a28a2949","unresolved":false,"context_lines":[{"line_number":856,"context_line":"* -1 means unlimited"},{"line_number":857,"context_line":"* Any integer \u003e\u003d 0 represents the maximum allowed"},{"line_number":858,"context_line":"\"\"\"),"},{"line_number":859,"context_line":"    cfg.IntOpt(\u0027volume_detach_retry_count\u0027,"},{"line_number":860,"context_line":"        default\u003d5,"},{"line_number":861,"context_line":"        min\u003d1,"},{"line_number":862,"context_line":"        help\u003d\"\"\""}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_e811cbb0","line":859,"updated":"2019-12-05 15:04:04.000000000","message":"If we\u0027re going to have a new config option, we should probably at least have a short \u0027fixes\u0027 release note mentioning the new config option.\n\nIt would also be nice to have a follow up change that is not backported which consolidates the generic option with the existing hyper-v specific ones I mentioned earlier:\n\nhttps://docs.openstack.org/nova/latest/configuration/config.html#hyperv.volume_attach_retry_count\n\nhttps://docs.openstack.org/nova/latest/configuration/config.html#hyperv.volume_attach_retry_interval\n\nI realize this option is for detach while those are for attach, but I would think we could just have generic options for both.","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c4223dc03b57787e9b1d792d98d99b0ded955ea2","unresolved":false,"context_lines":[{"line_number":860,"context_line":"        default\u003d5,"},{"line_number":861,"context_line":"        min\u003d1,"},{"line_number":862,"context_line":"        help\u003d\"\"\""},{"line_number":863,"context_line":"Volume API detach call retry count"},{"line_number":864,"context_line":""},{"line_number":865,"context_line":"The number of times to retry detaching a volume. Volume detach"},{"line_number":866,"context_line":"is retried until success or the given retry count is reached."}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_88d5b7b9","line":863,"updated":"2019-12-05 15:07:50.000000000","message":"Technically this option isn\u0027t only used when detaching a volume. We terminate connections and delete attachments during move operations without actually detaching the volume. I\u0027m not sure that\u0027s really worth mentioning in the docs here though since it probably doesn\u0027t matter.","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c4223dc03b57787e9b1d792d98d99b0ded955ea2","unresolved":false,"context_lines":[{"line_number":867,"context_line":""},{"line_number":868,"context_line":"Possible values:"},{"line_number":869,"context_line":""},{"line_number":870,"context_line":"* Positive integer values (Default: 5)."},{"line_number":871,"context_line":"\"\"\")"},{"line_number":872,"context_line":"]"},{"line_number":873,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_a87c33ea","line":870,"range":{"start_line":870,"start_character":26,"end_line":870,"end_character":38},"updated":"2019-12-05 15:07:50.000000000","message":"You don\u0027t need to mention this, it will be rendered in the docs automatically:\n\nhttps://storage.bhs1.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_52b/669674/9/check/openstack-tox-docs/52ba01a/docs/configuration/config.html#compute.volume_detach_retry_count","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"19275220b055c5ec14fd2ec75f89b6b0f75157f9","unresolved":false,"context_lines":[{"line_number":868,"context_line":"Possible values:"},{"line_number":869,"context_line":""},{"line_number":870,"context_line":"* Positive integer values (Default: 5)."},{"line_number":871,"context_line":"\"\"\")"},{"line_number":872,"context_line":"]"},{"line_number":873,"context_line":""},{"line_number":874,"context_line":"interval_opts \u003d ["}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_28eda3b7","line":871,"updated":"2019-12-05 15:07:39.000000000","message":"Looks like lyarwood asked for this at [1]. I\u0027m not sure why it would be necessary though. If something is failing to complete after 5 loops, surely that\u0027s an issue that bumping the value will only mask?\n\n[1] https://review.opendev.org/#/c/669674/3/nova/compute/manager.py@2613","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"}],"nova/tests/unit/compute/test_compute_mgr.py":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"744f3d2c85cf4b171e2182ba5b302513a8ac6e22","unresolved":false,"context_lines":[{"line_number":1419,"context_line":"    @mock.patch(\u0027nova.volume.cinder.API.attachment_delete\u0027)"},{"line_number":1420,"context_line":"    def test_volume_detach_delete_client_exception(self,"},{"line_number":1421,"context_line":"                                                   mock_attachment_delete):"},{"line_number":1422,"context_line":"        mock_attachment_delete.side_effect \u003d cinder_exception.\\"},{"line_number":1423,"context_line":"                                                      ClientException(code\u003d500)"},{"line_number":1424,"context_line":""},{"line_number":1425,"context_line":"        bdm \u003d mock.Mock(attachment_id\u003d1, is_volume\u003dTrue)"},{"line_number":1426,"context_line":"        fake_connector \u003d mock.sentinel.fake_connector"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa7e38b_24d552ba","line":1423,"range":{"start_line":1422,"start_character":0,"end_line":1423,"end_character":79},"updated":"2019-09-16 15:12:04.000000000","message":"mock_attachment_delete.side_effect \u003d \\\n    cinder_exception.ClientException(code\u003d500)\n\nCan you also test the behaviour of a non-ClientException here, IOW that we only try once and then fail?","commit_id":"9f16721708097e4bc3fe8302deca56507fd965fa"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"744f3d2c85cf4b171e2182ba5b302513a8ac6e22","unresolved":false,"context_lines":[{"line_number":1425,"context_line":"        bdm \u003d mock.Mock(attachment_id\u003d1, is_volume\u003dTrue)"},{"line_number":1426,"context_line":"        fake_connector \u003d mock.sentinel.fake_connector"},{"line_number":1427,"context_line":""},{"line_number":1428,"context_line":"        try:"},{"line_number":1429,"context_line":"            self.compute._volume_detach_delete(self.context, bdm,"},{"line_number":1430,"context_line":"                                               fake_connector)"},{"line_number":1431,"context_line":"        except cinder_exception.ClientException:"},{"line_number":1432,"context_line":"            pass"},{"line_number":1433,"context_line":""},{"line_number":1434,"context_line":"        self.assertEqual(CONF.volume_detach_retry_count,"},{"line_number":1435,"context_line":"                         mock_attachment_delete.call_count)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa7e38b_649fcab4","line":1432,"range":{"start_line":1428,"start_character":0,"end_line":1432,"end_character":16},"updated":"2019-09-16 15:12:04.000000000","message":"Yse assertRaises here to avoid this:\n\nhttps://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises","commit_id":"9f16721708097e4bc3fe8302deca56507fd965fa"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"655ad301fd44464087404ed55b4f014d63903362","unresolved":false,"context_lines":[{"line_number":1417,"context_line":"                20)"},{"line_number":1418,"context_line":""},{"line_number":1419,"context_line":"    @mock.patch(\u0027nova.volume.cinder.API.attachment_delete\u0027)"},{"line_number":1420,"context_line":"    def _test_volume_detach_delete_exception(self, exc, num_retry,"},{"line_number":1421,"context_line":"                                             mock_attachment_delete):"},{"line_number":1422,"context_line":"        mock_attachment_delete.side_effect \u003d exc"},{"line_number":1423,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_b6d2bb33","line":1420,"range":{"start_line":1420,"start_character":56,"end_line":1420,"end_character":65},"updated":"2019-09-23 08:46:39.000000000","message":"expected_retry_count?","commit_id":"613f2162989082f72dd281b9786ec834cc92a759"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"655ad301fd44464087404ed55b4f014d63903362","unresolved":false,"context_lines":[{"line_number":1424,"context_line":"        bdm \u003d mock.Mock(attachment_id\u003d1, is_volume\u003dTrue)"},{"line_number":1425,"context_line":"        instance \u003d fake_instance.fake_instance_obj(self.context)"},{"line_number":1426,"context_line":""},{"line_number":1427,"context_line":"        self.assertRaises(type(exc), self.compute._volume_detach_delete,"},{"line_number":1428,"context_line":"                          self.context, bdm, instance)"},{"line_number":1429,"context_line":""},{"line_number":1430,"context_line":"        self.assertEqual(num_retry, mock_attachment_delete.call_count)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_364a4b27","line":1427,"range":{"start_line":1427,"start_character":26,"end_line":1427,"end_character":35},"updated":"2019-09-23 08:46:39.000000000","message":"This can just be exc right? I\u0027d personally rename it to raised_expection or something more meaningful as well.","commit_id":"613f2162989082f72dd281b9786ec834cc92a759"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"655ad301fd44464087404ed55b4f014d63903362","unresolved":false,"context_lines":[{"line_number":1431,"context_line":""},{"line_number":1432,"context_line":"    def test_volume_detach_delete_client_exception(self):"},{"line_number":1433,"context_line":"        exc \u003d cinder_exception.ClientException(code\u003d500)"},{"line_number":1434,"context_line":"        num_retry \u003d CONF.volume_detach_retry_count"},{"line_number":1435,"context_line":"        self._test_volume_detach_delete_exception(exc, num_retry)"},{"line_number":1436,"context_line":""},{"line_number":1437,"context_line":"    def test_volume_detach_delete_other_exception(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_f61a7315","line":1434,"range":{"start_line":1434,"start_character":20,"end_line":1434,"end_character":50},"updated":"2019-09-23 08:46:39.000000000","message":"This isn\u0027t correct, it should be `CONF.compute.volume_detach_retry_count`.\n\nRegardless, we would normally set this within the test itself using \n\n    self.flags(volume_detach_retry_count\u003d5, group\u003d\u0027compute\u0027)","commit_id":"613f2162989082f72dd281b9786ec834cc92a759"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"655ad301fd44464087404ed55b4f014d63903362","unresolved":false,"context_lines":[{"line_number":1435,"context_line":"        self._test_volume_detach_delete_exception(exc, num_retry)"},{"line_number":1436,"context_line":""},{"line_number":1437,"context_line":"    def test_volume_detach_delete_other_exception(self):"},{"line_number":1438,"context_line":"        exc \u003d Exception(\u0027some other exception\u0027)"},{"line_number":1439,"context_line":"        num_retry \u003d 1"},{"line_number":1440,"context_line":"        self._test_volume_detach_delete_exception(exc, num_retry)"},{"line_number":1441,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_16edcfee","line":1438,"range":{"start_line":1438,"start_character":8,"end_line":1438,"end_character":47},"updated":"2019-09-23 08:46:39.000000000","message":"You could also provide this in the call below.","commit_id":"613f2162989082f72dd281b9786ec834cc92a759"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"655ad301fd44464087404ed55b4f014d63903362","unresolved":false,"context_lines":[{"line_number":1436,"context_line":""},{"line_number":1437,"context_line":"    def test_volume_detach_delete_other_exception(self):"},{"line_number":1438,"context_line":"        exc \u003d Exception(\u0027some other exception\u0027)"},{"line_number":1439,"context_line":"        num_retry \u003d 1"},{"line_number":1440,"context_line":"        self._test_volume_detach_delete_exception(exc, num_retry)"},{"line_number":1441,"context_line":""},{"line_number":1442,"context_line":"    @mock.patch(\u0027nova.context.RequestContext.elevated\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa7e38b_f6efd3e6","line":1439,"range":{"start_line":1439,"start_character":0,"end_line":1439,"end_character":21},"updated":"2019-09-23 08:46:39.000000000","message":"nit - I don\u0027t think you need a variable here.","commit_id":"613f2162989082f72dd281b9786ec834cc92a759"}],"nova/tests/unit/volume/test_cinder.py":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"a0f9b62723381becd6bb9bf2da6a6138a76c0e30","unresolved":false,"context_lines":[{"line_number":537,"context_line":"        mock_cinderclient.assert_called_once_with(self.ctx, \u00273.44\u0027,"},{"line_number":538,"context_line":"                                                  skip_version_check\u003dTrue)"},{"line_number":539,"context_line":""},{"line_number":540,"context_line":"    @mock.patch(\u0027nova.volume.cinder.cinderclient\u0027,"},{"line_number":541,"context_line":"                side_effect\u003dcinder_exception.ClientException(code\u003d500))"},{"line_number":542,"context_line":"    def test_attachment_delete_client_exception(self,"},{"line_number":543,"context_line":"                                                mock_cinderclient):"},{"line_number":544,"context_line":""},{"line_number":545,"context_line":"        self.flags(volume_detach_retry_count\u003d5, group\u003d\u0027compute\u0027)"},{"line_number":546,"context_line":""},{"line_number":547,"context_line":"        self.assertRaises(cinder_exception.ClientException,"},{"line_number":548,"context_line":"                          self.api.attachment_delete,"},{"line_number":549,"context_line":"                          self.ctx, uuids.attachment_id)"},{"line_number":550,"context_line":""},{"line_number":551,"context_line":"        self.assertEqual(5, mock_cinderclient.call_count)"},{"line_number":552,"context_line":""},{"line_number":553,"context_line":"    @mock.patch(\u0027nova.volume.cinder.cinderclient\u0027,"},{"line_number":554,"context_line":"                side_effect\u003dcinder_exception.BadRequest(code\u003d400))"},{"line_number":555,"context_line":"    def test_attachment_delete_bad_request_exception(self,"},{"line_number":556,"context_line":"                                                mock_cinderclient):"},{"line_number":557,"context_line":""},{"line_number":558,"context_line":"        self.flags(volume_detach_retry_count\u003d5, group\u003d\u0027compute\u0027)"},{"line_number":559,"context_line":""},{"line_number":560,"context_line":"        self.assertRaises(exception.InvalidInput,"},{"line_number":561,"context_line":"                          self.api.attachment_delete,"},{"line_number":562,"context_line":"                          self.ctx, uuids.attachment_id)"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"        self.assertEqual(1, mock_cinderclient.call_count)"},{"line_number":565,"context_line":""},{"line_number":566,"context_line":"    @mock.patch(\u0027nova.volume.cinder.cinderclient\u0027)"},{"line_number":567,"context_line":"    def test_attachment_complete(self, mock_cinderclient):"}],"source_content_type":"text/x-python","patch_set":8,"id":"3fa7e38b_8359715c","line":564,"range":{"start_line":540,"start_character":0,"end_line":564,"end_character":57},"updated":"2019-11-08 11:48:15.000000000","message":"LGTM but it would be nice to see a test for the positive case where the first call fails but subsequent attempts pass. Just provide an iterable as a side_effect of [cinder_exception.ClientException(code\u003d500), None] and assert that the client is called twice without an exception being raised:\n\nhttps://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.side_effect","commit_id":"2c69428553e6c2bbda56b48827814a0353417ca1"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"19275220b055c5ec14fd2ec75f89b6b0f75157f9","unresolved":false,"context_lines":[{"line_number":551,"context_line":"        self.assertEqual(5, mock_cinderclient.call_count)"},{"line_number":552,"context_line":""},{"line_number":553,"context_line":"    @mock.patch(\u0027nova.volume.cinder.cinderclient\u0027)"},{"line_number":554,"context_line":"    def test_attachment_delete_client_exception_do_not_raise(self,"},{"line_number":555,"context_line":"                                                         mock_cinderclient):"},{"line_number":556,"context_line":"        self.flags(volume_detach_retry_count\u003d5, group\u003d\u0027compute\u0027)"},{"line_number":557,"context_line":""},{"line_number":558,"context_line":"        # generate exception, and then have a normal return on the next retry"}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_a8075370","line":555,"range":{"start_line":554,"start_character":61,"end_line":555,"end_character":74},"updated":"2019-12-05 15:07:39.000000000","message":"style nit: just drag both of these onto the same line. Ditto below","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"19275220b055c5ec14fd2ec75f89b6b0f75157f9","unresolved":false,"context_lines":[{"line_number":556,"context_line":"        self.flags(volume_detach_retry_count\u003d5, group\u003d\u0027compute\u0027)"},{"line_number":557,"context_line":""},{"line_number":558,"context_line":"        # generate exception, and then have a normal return on the next retry"},{"line_number":559,"context_line":"        mock_cinderclient.return_value.attachments.delete.side_effect\\"},{"line_number":560,"context_line":"            \u003d [cinder_exception.ClientException(code\u003d500), None]"},{"line_number":561,"context_line":""},{"line_number":562,"context_line":"        attachment_id \u003d uuids.attachment"},{"line_number":563,"context_line":"        self.api.attachment_delete(self.ctx, attachment_id)"}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_48181fcc","line":560,"range":{"start_line":559,"start_character":69,"end_line":560,"end_character":15},"updated":"2019-12-05 15:07:39.000000000","message":"nit: Try\n\n ...side_effect \u003d [\n      cinder_exception.ClientException(code\u003d500), None]","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"}],"nova/volume/cinder.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c4223dc03b57787e9b1d792d98d99b0ded955ea2","unresolved":false,"context_lines":[{"line_number":546,"context_line":"                                             mountpoint, mode\u003dmode)"},{"line_number":547,"context_line":""},{"line_number":548,"context_line":"    @translate_volume_exception"},{"line_number":549,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d"},{"line_number":550,"context_line":"                        CONF.compute.volume_detach_retry_count,"},{"line_number":551,"context_line":"                    retry_on_exception\u003dlambda e:"},{"line_number":552,"context_line":"                        (type(e) \u003d\u003d cinder_exception.ClientException))"}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_e8ed4b8e","line":549,"updated":"2019-12-05 15:07:50.000000000","message":"Is there a backoff on this with the retry interval or are we just retrying again immediately?","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"19275220b055c5ec14fd2ec75f89b6b0f75157f9","unresolved":false,"context_lines":[{"line_number":546,"context_line":"                                             mountpoint, mode\u003dmode)"},{"line_number":547,"context_line":""},{"line_number":548,"context_line":"    @translate_volume_exception"},{"line_number":549,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d"},{"line_number":550,"context_line":"                        CONF.compute.volume_detach_retry_count,"},{"line_number":551,"context_line":"                    retry_on_exception\u003dlambda e:"},{"line_number":552,"context_line":"                        (type(e) \u003d\u003d cinder_exception.ClientException))"},{"line_number":553,"context_line":"    def detach(self, context, volume_id, instance_uuid\u003dNone,"},{"line_number":554,"context_line":"               attachment_id\u003dNone):"},{"line_number":555,"context_line":"        client \u003d cinderclient(context)"}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_8867f751","line":552,"range":{"start_line":549,"start_character":20,"end_line":552,"end_character":69},"updated":"2019-12-05 15:07:39.000000000","message":"style nit: just drag these down onto new lines and lose the wrapping","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"19275220b055c5ec14fd2ec75f89b6b0f75157f9","unresolved":false,"context_lines":[{"line_number":549,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d"},{"line_number":550,"context_line":"                        CONF.compute.volume_detach_retry_count,"},{"line_number":551,"context_line":"                    retry_on_exception\u003dlambda e:"},{"line_number":552,"context_line":"                        (type(e) \u003d\u003d cinder_exception.ClientException))"},{"line_number":553,"context_line":"    def detach(self, context, volume_id, instance_uuid\u003dNone,"},{"line_number":554,"context_line":"               attachment_id\u003dNone):"},{"line_number":555,"context_line":"        client \u003d cinderclient(context)"}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_08654745","line":552,"range":{"start_line":552,"start_character":24,"end_line":552,"end_character":25},"updated":"2019-12-05 15:07:39.000000000","message":"these brackets look unnecessary?","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"19275220b055c5ec14fd2ec75f89b6b0f75157f9","unresolved":false,"context_lines":[{"line_number":616,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d"},{"line_number":617,"context_line":"                        CONF.compute.volume_detach_retry_count,"},{"line_number":618,"context_line":"                    retry_on_exception\u003dlambda e:"},{"line_number":619,"context_line":"                        (type(e) \u003d\u003d cinder_exception.ClientException))"},{"line_number":620,"context_line":"    def terminate_connection(self, context, volume_id, connector):"},{"line_number":621,"context_line":"        return cinderclient(context).volumes.terminate_connection(volume_id,"},{"line_number":622,"context_line":"                                                                  connector)"}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_a8551354","line":619,"updated":"2019-12-05 15:07:39.000000000","message":"ditto","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7ad65b1b9b3fc90bf0c9589a1995b16feca93a20","unresolved":false,"context_lines":[{"line_number":866,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d"},{"line_number":867,"context_line":"                        CONF.compute.volume_detach_retry_count,"},{"line_number":868,"context_line":"                    retry_on_exception\u003dlambda e:"},{"line_number":869,"context_line":"                        (type(e) \u003d\u003d cinder_exception.ClientException))"},{"line_number":870,"context_line":"    def attachment_delete(self, context, attachment_id):"},{"line_number":871,"context_line":"        try:"},{"line_number":872,"context_line":"            cinderclient("}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_e8bb0b72","line":869,"updated":"2019-12-05 15:10:59.000000000","message":"This seems extremely broad.\n\nLet\u0027s say we get a MessagingTimeout in cinder which results in a 500 error. But the actual backend in cinder has deleted the attachment. Now we retry and this time we get a 400 response from Cinder because the resource is already gone, we\u0027ll just continue retrying until we exhaust our retries and raise that NotFound error back up to the caller, right?","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c467f38997146d833444fd31eb39db7e7337a70d","unresolved":false,"context_lines":[{"line_number":866,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d"},{"line_number":867,"context_line":"                        CONF.compute.volume_detach_retry_count,"},{"line_number":868,"context_line":"                    retry_on_exception\u003dlambda e:"},{"line_number":869,"context_line":"                        (type(e) \u003d\u003d cinder_exception.ClientException))"},{"line_number":870,"context_line":"    def attachment_delete(self, context, attachment_id):"},{"line_number":871,"context_line":"        try:"},{"line_number":872,"context_line":"            cinderclient("}],"source_content_type":"text/x-python","patch_set":9,"id":"3fa7e38b_685ddb46","line":869,"in_reply_to":"3fa7e38b_e8bb0b72","updated":"2019-12-05 15:14:49.000000000","message":"Could we scope this to only ClientException where the code is 500?","commit_id":"862a90d304110d33c4fd53768c50435a44c54d73"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"c88f5e56ae84bfc4b4fa583a2b7ba8960bd7f9aa","unresolved":false,"context_lines":[{"line_number":549,"context_line":"    @translate_volume_exception"},{"line_number":550,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d5,"},{"line_number":551,"context_line":"                    retry_on_exception\u003dlambda e:"},{"line_number":552,"context_line":"                    type(e) \u003d\u003d cinder_apiclient.exceptions.InternalServerError)"},{"line_number":553,"context_line":"    def detach(self, context, volume_id, instance_uuid\u003dNone,"},{"line_number":554,"context_line":"               attachment_id\u003dNone):"},{"line_number":555,"context_line":"        client \u003d cinderclient(context)"}],"source_content_type":"text/x-python","patch_set":10,"id":"1fa4df85_ad26e567","line":552,"range":{"start_line":552,"start_character":20,"end_line":552,"end_character":78},"updated":"2020-03-12 17:50:41.000000000","message":"Any reason not to use:\n\n  isinstance(e, cinder_apiclient.exceptions.InternalServerError)\n\n?\n\nI don\u0027t know if cinder subclasses \u0027InternalServerError\u0027 but if it does, \u0027type(e) \u003d\u003d\u0027 won\u0027t catch things","commit_id":"01c334cbdd859f4e486ac2c369a4bdb3ec7709cc"},{"author":{"_account_id":30009,"name":"François Palin","email":"fpalin@redhat.com","username":"FrancoisPalin"},"change_message_id":"5ea2a07daa106b6e721abada48f06a96ee5abf08","unresolved":false,"context_lines":[{"line_number":549,"context_line":"    @translate_volume_exception"},{"line_number":550,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d5,"},{"line_number":551,"context_line":"                    retry_on_exception\u003dlambda e:"},{"line_number":552,"context_line":"                    type(e) \u003d\u003d cinder_apiclient.exceptions.InternalServerError)"},{"line_number":553,"context_line":"    def detach(self, context, volume_id, instance_uuid\u003dNone,"},{"line_number":554,"context_line":"               attachment_id\u003dNone):"},{"line_number":555,"context_line":"        client \u003d cinderclient(context)"}],"source_content_type":"text/x-python","patch_set":10,"id":"1fa4df85_b96859ba","line":552,"range":{"start_line":552,"start_character":20,"end_line":552,"end_character":78},"in_reply_to":"1fa4df85_ad26e567","updated":"2020-03-13 14:09:56.000000000","message":"I am specifically (and intentionally) targeting InternalServerError (HTTP 500) to trigger a retry, and not its subclasses","commit_id":"01c334cbdd859f4e486ac2c369a4bdb3ec7709cc"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"bdb03540bfa3e5c9b79f22289090f08529e35e96","unresolved":false,"context_lines":[{"line_number":549,"context_line":"    @translate_volume_exception"},{"line_number":550,"context_line":"    @retrying.retry(stop_max_attempt_number\u003d5,"},{"line_number":551,"context_line":"                    retry_on_exception\u003dlambda e:"},{"line_number":552,"context_line":"                    type(e) \u003d\u003d cinder_apiclient.exceptions.InternalServerError)"},{"line_number":553,"context_line":"    def detach(self, context, volume_id, instance_uuid\u003dNone,"},{"line_number":554,"context_line":"               attachment_id\u003dNone):"},{"line_number":555,"context_line":"        client \u003d cinderclient(context)"}],"source_content_type":"text/x-python","patch_set":10,"id":"df33271e_c73de9bb","line":552,"range":{"start_line":552,"start_character":20,"end_line":552,"end_character":78},"in_reply_to":"1fa4df85_b96859ba","updated":"2020-04-02 15:29:43.000000000","message":"Ah, fair. Thanks","commit_id":"01c334cbdd859f4e486ac2c369a4bdb3ec7709cc"}]}