)]}'
{"nova/compute/manager.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"10cf1e20c528f378d74ff57ccd9014bc8a0a4059","unresolved":false,"context_lines":[{"line_number":4774,"context_line":"        self._driver_detach_volume(context, instance, bdm)"},{"line_number":4775,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4776,"context_line":""},{"line_number":4777,"context_line":"        # NOTE(lyarwood): ignore failures by cinder to terminate a volumes"},{"line_number":4778,"context_line":"        # connection as the connector information provide by the compute driver"},{"line_number":4779,"context_line":"        # might related to a different host during an evacuation."},{"line_number":4780,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba63e191_fc78b191","line":4777,"updated":"2016-01-18 18:15:56.000000000","message":"nit: volume\u0027s","commit_id":"dc130322e00989d7a217bdfafe3f47624ea022d3"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"0f3292e8b60a330f442e2a571caac27bb5b8c0e4","unresolved":false,"context_lines":[{"line_number":4774,"context_line":"        self._driver_detach_volume(context, instance, bdm)"},{"line_number":4775,"context_line":"        connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":4776,"context_line":""},{"line_number":4777,"context_line":"        # NOTE(lyarwood): ignore failures by cinder to terminate a volumes"},{"line_number":4778,"context_line":"        # connection as the connector information provide by the compute driver"},{"line_number":4779,"context_line":"        # might related to a different host during an evacuation."},{"line_number":4780,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba63e191_ae03bc0e","line":4777,"in_reply_to":"ba63e191_fc78b191","updated":"2016-01-19 10:21:43.000000000","message":"ACK, thanks!","commit_id":"dc130322e00989d7a217bdfafe3f47624ea022d3"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"10cf1e20c528f378d74ff57ccd9014bc8a0a4059","unresolved":false,"context_lines":[{"line_number":4779,"context_line":"        # might related to a different host during an evacuation."},{"line_number":4780,"context_line":"        try:"},{"line_number":4781,"context_line":"            self.volume_api.terminate_connection(context, volume_id, connector)"},{"line_number":4782,"context_line":"        except cinder_exception.ClientException as exc:"},{"line_number":4783,"context_line":"            LOG.warning(_LW(\u0027Ignoring cinder client exception %s\u0027"},{"line_number":4784,"context_line":"                            \u0027while attempting to terminate a connection to \u0027"},{"line_number":4785,"context_line":"                            \u0027volume %s.\u0027), exc, volume_id, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba63e191_57dd166f","line":4782,"updated":"2016-01-18 18:15:56.000000000","message":"Does terminate_connection do anything persistent in cinder if the connection doesn\u0027t exist? e.g. Does it cleanup any state?\n\nWhere am I going with this? It seems to me that we should know in advance from the calling context that this isn\u0027t going to work. Basically, if we\u0027re rebuilding on a host which isn\u0027t the original host, this is going to raise an exception, right? If terminate_connection does other stuff, I guess we need to call it anyway and catch the exception, and that would be an api wart in cinder. If it doesn\u0027t do anything else, though, we just shouldn\u0027t call it at all. Looks like we\u0027re called from _do_rebuild_instance(), which should know whether we\u0027re on the original host or not. Can we handle it there?","commit_id":"dc130322e00989d7a217bdfafe3f47624ea022d3"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"0f3292e8b60a330f442e2a571caac27bb5b8c0e4","unresolved":false,"context_lines":[{"line_number":4779,"context_line":"        # might related to a different host during an evacuation."},{"line_number":4780,"context_line":"        try:"},{"line_number":4781,"context_line":"            self.volume_api.terminate_connection(context, volume_id, connector)"},{"line_number":4782,"context_line":"        except cinder_exception.ClientException as exc:"},{"line_number":4783,"context_line":"            LOG.warning(_LW(\u0027Ignoring cinder client exception %s\u0027"},{"line_number":4784,"context_line":"                            \u0027while attempting to terminate a connection to \u0027"},{"line_number":4785,"context_line":"                            \u0027volume %s.\u0027), exc, volume_id, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba63e191_6104f514","line":4782,"in_reply_to":"ba63e191_57dd166f","updated":"2016-01-19 10:21:43.000000000","message":"No, the volume implementations I\u0027ve reviewed just try to remove access to the volume based on the connector data provided and that\u0027s it. If this data is invalid  they either return an error or swallow it on the Cinder side and return as normal.\n\nWe could try to make an educated guess further up in _do_rebuild_instance but it would be extremely difficult to get right given the number compute and volume drivers in play here.\n\nBy ignoring failures here we either leave access to the volume open from the current host where the instance is about to be rebuilt _or_ access from the previous host that we are likely evacuating from. IMHO in the short term this is acceptable.\n\nThe longer term fix would be to ensure that the connector data used by the original host to connect to the volume is persisted with the instance. This way we can be sure that we are terminating the correct connection at this point in the rebuild process.","commit_id":"dc130322e00989d7a217bdfafe3f47624ea022d3"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"1eec64fdf916a2207fae4c2682d072a263ca1a9d","unresolved":false,"context_lines":[{"line_number":4779,"context_line":"        # might related to a different host during an evacuation."},{"line_number":4780,"context_line":"        try:"},{"line_number":4781,"context_line":"            self.volume_api.terminate_connection(context, volume_id, connector)"},{"line_number":4782,"context_line":"        except cinder_exception.ClientException as exc:"},{"line_number":4783,"context_line":"            LOG.warning(_LW(\u0027Ignoring cinder client exception %s\u0027"},{"line_number":4784,"context_line":"                            \u0027while attempting to terminate a connection to \u0027"},{"line_number":4785,"context_line":"                            \u0027volume %s.\u0027), exc, volume_id, instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba63e191_412bf1eb","line":4782,"in_reply_to":"ba63e191_6104f514","updated":"2016-01-19 10:53:25.000000000","message":"Ok, I agree with that. I looked at a few implementations of get_volume_connector in nova virt drivers, and I don\u0027t think we can assume we can reconstruct that from the new compute. Ideally we\u0027d need to way to reconstruct it from the new node or, as you suggest, store it with the volume. This is going to leave untidy ACLs and that sucks, but that\u0027s how it is now anyway. Maybe a note describing the outline solution, though? At least a note about the implications of not being able to successfully call terminate_connection().\n\nHowever, I\u0027m still not comfortable with catching ClientException here. If we know that this can\u0027t do anything useful because the connection info is definitely wrong, we shouldn\u0027t call it in the first place. We do have that context in _do_rebuild_instance, and we could pass it into this function.\n\nEven if we\u0027re going to call it anyway and catch an exception, we need to catch the most specific exception possible. Right now we\u0027re ignoring any cinder exception, and that\u0027s not ideal. I could be convinced this is impractical due to ugly implementations, but if so it should come with a comment explaining why.","commit_id":"dc130322e00989d7a217bdfafe3f47624ea022d3"}]}