)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"836201540f63e48faa0595444649796b4223010e","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Check for outstanding attachments during reserve"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There are a number of places on the Nova side where we need to do things"},{"line_number":10,"context_line":"with volume connections like remove an attachment and immediately"},{"line_number":11,"context_line":"reserve a new one. This introduces a potential race where bad things"},{"line_number":12,"context_line":"can happen. To deal with that, we actually want to to create a new"},{"line_number":13,"context_line":"attachment to do a reserve, BEFORE we delete the current attachment."},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"5f2577a7_5980b2bf","line":11,"range":{"start_line":9,"start_character":0,"end_line":11,"end_character":19},"updated":"2017-09-16 14:12:11.000000000","message":"On the nova side, I think what we want to do is:\n\n1. create new empty attachment (no connector), this keeps the volume reserved\n2. delete the old attachment\n\nIn what scenarios do we need to delete the old attachment and then immediately create a new attachment (which exposes this race window)?","commit_id":"a169452f8f0446b427b3958f2daabf885b115f88"},{"author":{"_account_id":2243,"name":"John Griffith","email":"john.griffith8@gmail.com","username":"john-griffith"},"change_message_id":"006dff283187b71922f50ac75054912f9aeb9ac5","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Check for outstanding attachments during reserve"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"There are a number of places on the Nova side where we need to do things"},{"line_number":10,"context_line":"with volume connections like remove an attachment and immediately"},{"line_number":11,"context_line":"reserve a new one. This introduces a potential race where bad things"},{"line_number":12,"context_line":"can happen. To deal with that, we actually want to to create a new"},{"line_number":13,"context_line":"attachment to do a reserve, BEFORE we delete the current attachment."},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"ff5d4b2f_00736fe9","line":11,"range":{"start_line":9,"start_character":0,"end_line":11,"end_character":19},"in_reply_to":"5f2577a7_5980b2bf","updated":"2017-09-19 13:23:27.000000000","message":"Yes, you\u0027re correct.  I had the other way of doing this in mind when writing that.  I\u0027ll get it fixed when I rework this.\n\nTanks!","commit_id":"a169452f8f0446b427b3958f2daabf885b115f88"}],"cinder/tests/unit/attachments/test_attachments_api.py":[{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"0d55a4a0e7f36e64577964dfd186b9356438dc58","unresolved":false,"context_lines":[{"line_number":167,"context_line":"        self.assertEqual(vref.id, aref.volume_id)"},{"line_number":168,"context_line":"        self.assertEqual({}, aref.connection_info)"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":"        self.assertRaises(exception.InvalidVolume,"},{"line_number":171,"context_line":"                          self.volume_api.attachment_create,"},{"line_number":172,"context_line":"                          self.context,"},{"line_number":173,"context_line":"                          vref,"},{"line_number":174,"context_line":"                          fake.UUID1)"},{"line_number":175,"context_line":"        self.volume_api.attachment_create(self.context,"},{"line_number":176,"context_line":"                                          vref,"},{"line_number":177,"context_line":"                                          fake.UUID2)"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f2577a7_cfff30a9","side":"PARENT","line":174,"range":{"start_line":170,"start_character":8,"end_line":174,"end_character":37},"updated":"2017-09-16 16:50:49.000000000","message":"I would keep the attachment_create with fake.UUID1 and bump the number of attachments to 3 in the last line so we test the option with different instance uuid .","commit_id":"def3cb7f043e1d7d6c37e7c9b916878ffcd0b93c"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"0d55a4a0e7f36e64577964dfd186b9356438dc58","unresolved":false,"context_lines":[{"line_number":15,"context_line":""},{"line_number":16,"context_line":"from cinder import context"},{"line_number":17,"context_line":"from cinder import db"},{"line_number":18,"context_line":"from cinder import exception"},{"line_number":19,"context_line":"from cinder import objects"},{"line_number":20,"context_line":"from cinder import test"},{"line_number":21,"context_line":"from cinder.tests.unit import fake_constants as fake"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f2577a7_2f2a342b","line":18,"range":{"start_line":18,"start_character":0,"end_line":18,"end_character":28},"updated":"2017-09-16 16:50:49.000000000","message":"pep8 cries as this is now an unused import.","commit_id":"211cc2c0e336ecb471a84e94b3101635aa726ae9"}],"cinder/volume/api.py":[{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"90fbf358c2f30c105b5c5b107152236196acd759","unresolved":false,"context_lines":[{"line_number":1953,"context_line":"        # in use.  We do this in Nova to address races in some of the"},{"line_number":1954,"context_line":"        # special cases like live-migration etc.  So to deal with"},{"line_number":1955,"context_line":"        # that we want to ignore in-use and reserved status here"},{"line_number":1956,"context_line":"        if vref.status not in [\u0027in-use\u0027, \u0027reserved\u0027]"},{"line_number":1957,"context_line":"            expected \u003d {\u0027status\u0027: (\u0027available\u0027, \u0027downloading\u0027)}"},{"line_number":1958,"context_line":"            result \u003d vref.conditional_update({\u0027status\u0027: \u0027reserved\u0027}, expected)"},{"line_number":1959,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5f2577a7_aa673ab7","line":1956,"range":{"start_line":1956,"start_character":51,"end_line":1956,"end_character":52},"updated":"2017-09-15 20:58:26.000000000","message":"\u0027:\u0027 is missing here.","commit_id":"afe95fb923007c18039c91027727e59f28a0ee2f"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"0d55a4a0e7f36e64577964dfd186b9356438dc58","unresolved":false,"context_lines":[{"line_number":2041,"context_line":"                                                 attachment.id,"},{"line_number":2042,"context_line":"                                                 volume)"},{"line_number":2043,"context_line":"        remaining_attachments \u003d AO_LIST.get_all_by_volume_id(ctxt, volume.id)"},{"line_number":2044,"context_line":"        for att in remaining_attachments:"},{"line_number":2045,"context_line":"            if att.attach_status \u003d\u003d fields.VolumeAttachStatus.RESERVED:"},{"line_number":2046,"context_line":"                volume.attach_status \u003d fields.VolumeAttachStatus.RESERVED"},{"line_number":2047,"context_line":"                volume.status \u003d \u0027reserved\u0027"},{"line_number":2048,"context_line":"                volume.save()"},{"line_number":2049,"context_line":""},{"line_number":2050,"context_line":"        # TODO(jdg): Make this check attachments_by_volume_id when we"},{"line_number":2051,"context_line":"        # implement multi-attach for real"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f2577a7_ef92ec56","line":2048,"range":{"start_line":2044,"start_character":8,"end_line":2048,"end_character":29},"updated":"2017-09-16 16:50:49.000000000","message":"One thing we haven\u0027t thought about. We should put the volume into \u0027reserved\u0027 state only if there are only attachments in \u0027reserved\u0027 state left. If we have finalized attachments it should still be \u0027in-use\u0027 so we can detach those.","commit_id":"211cc2c0e336ecb471a84e94b3101635aa726ae9"},{"author":{"_account_id":2243,"name":"John Griffith","email":"john.griffith8@gmail.com","username":"john-griffith"},"change_message_id":"6531ff3640c45deed860f2c998623afac91d46d1","unresolved":false,"context_lines":[{"line_number":2041,"context_line":"                                                 attachment.id,"},{"line_number":2042,"context_line":"                                                 volume)"},{"line_number":2043,"context_line":"        remaining_attachments \u003d AO_LIST.get_all_by_volume_id(ctxt, volume.id)"},{"line_number":2044,"context_line":"        for att in remaining_attachments:"},{"line_number":2045,"context_line":"            if att.attach_status \u003d\u003d fields.VolumeAttachStatus.RESERVED:"},{"line_number":2046,"context_line":"                volume.attach_status \u003d fields.VolumeAttachStatus.RESERVED"},{"line_number":2047,"context_line":"                volume.status \u003d \u0027reserved\u0027"},{"line_number":2048,"context_line":"                volume.save()"},{"line_number":2049,"context_line":""},{"line_number":2050,"context_line":"        # TODO(jdg): Make this check attachments_by_volume_id when we"},{"line_number":2051,"context_line":"        # implement multi-attach for real"}],"source_content_type":"text/x-python","patch_set":4,"id":"ff5d4b2f_5b403c08","line":2048,"range":{"start_line":2044,"start_character":8,"end_line":2048,"end_character":29},"in_reply_to":"5f2577a7_ef92ec56","updated":"2017-09-19 13:33:13.000000000","message":"Actually I did think of that :)\n\nBut this code is a bit of a mess now because I tried to rush :(.  \n\nI have an idea to possibly simplify all of this a little bit and will look at it later today to see if it works. I think we may be better off allowing the reserve in *any* state except maybe error states, and then decide what to do with the vol.status here based on greatest state of attachments.  In other words, priority is anything in-use, then reserved, then anything else.  Handling the \"xxxing\" states is the only thing I\u0027m sort of thinking about still.\n\nAlso, when we do multiattach we\u0027ll need to consider if we want to prevent attachment-update while another attachment-update (attaching) is in progress.  I think this should be *ok* but might be good to try and keep it sequential at least initially.","commit_id":"211cc2c0e336ecb471a84e94b3101635aa726ae9"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"bed0a67638a853ca6bd84e39104b29be0530be47","unresolved":false,"context_lines":[{"line_number":2041,"context_line":"                                                 attachment.id,"},{"line_number":2042,"context_line":"                                                 volume)"},{"line_number":2043,"context_line":"        remaining_attachments \u003d AO_LIST.get_all_by_volume_id(ctxt, volume.id)"},{"line_number":2044,"context_line":"        for att in remaining_attachments:"},{"line_number":2045,"context_line":"            if att.attach_status \u003d\u003d fields.VolumeAttachStatus.RESERVED:"},{"line_number":2046,"context_line":"                volume.attach_status \u003d fields.VolumeAttachStatus.RESERVED"},{"line_number":2047,"context_line":"                volume.status \u003d \u0027reserved\u0027"},{"line_number":2048,"context_line":"                volume.save()"},{"line_number":2049,"context_line":""},{"line_number":2050,"context_line":"        # TODO(jdg): Make this check attachments_by_volume_id when we"},{"line_number":2051,"context_line":"        # implement multi-attach for real"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf57530f_0d8a751a","line":2048,"range":{"start_line":2044,"start_character":8,"end_line":2048,"end_character":29},"in_reply_to":"ff5d4b2f_5b403c08","updated":"2017-09-20 18:38:25.000000000","message":"Ok, cool, we\u0027re on the right track then. :)\n\nAs this is the delete case we only need to decide on which state is the primary, where agree with you that if the volume still has any active attachments then it should be \u0027in-use\u0027. If it has only empty attachments left then \u0027reserved\u0027.\n\nAs for the \u0027detaching\u0027 case that\u0027s triggered by begin_detaching, which we will keep. In case the volume is in \u0027xxxing\u0027 state I don\u0027t think we should allow anything else to happen to it because of the shared connections case we talked about at the PTG. So my vote is on sequential.\n\nAllowing the reserve operation should not do any harm. We might want to look into some restrictions whether or not to allow it for any instance if it\u0027s not a multi-attach volume to avoid random reservations. But if the attachment_update handles that case properly it might not matter anyway.","commit_id":"211cc2c0e336ecb471a84e94b3101635aa726ae9"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"7725b68af399b06b8848ee8e0bdd0aa84ad142e9","unresolved":false,"context_lines":[{"line_number":1953,"context_line":"        # in use.  We do this in Nova to address races in some of the"},{"line_number":1954,"context_line":"        # special cases like live-migration etc.  So to deal with"},{"line_number":1955,"context_line":"        # that we want to ignore in-use and reserved status here"},{"line_number":1956,"context_line":"        if vref.status not in [\u0027in-use\u0027, \u0027reserved\u0027]:"},{"line_number":1957,"context_line":"            expected \u003d {\u0027status\u0027: (\u0027available\u0027, \u0027downloading\u0027)}"},{"line_number":1958,"context_line":"            result \u003d vref.conditional_update({\u0027status\u0027: \u0027reserved\u0027}, expected)"},{"line_number":1959,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"9f5c4f37_c588bf72","line":1956,"updated":"2017-09-21 14:16:27.000000000","message":"-1: With this implementation we could have a race condition right here because we are checking the status of vref we have in memory and not the one from the DB.","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":2243,"name":"John Griffith","email":"john.griffith8@gmail.com","username":"john-griffith"},"change_message_id":"58b3cca8d77ca941590276b2b88cfe27b3966cf4","unresolved":false,"context_lines":[{"line_number":1953,"context_line":"        # in use.  We do this in Nova to address races in some of the"},{"line_number":1954,"context_line":"        # special cases like live-migration etc.  So to deal with"},{"line_number":1955,"context_line":"        # that we want to ignore in-use and reserved status here"},{"line_number":1956,"context_line":"        if vref.status not in [\u0027in-use\u0027, \u0027reserved\u0027]:"},{"line_number":1957,"context_line":"            expected \u003d {\u0027status\u0027: (\u0027available\u0027, \u0027downloading\u0027)}"},{"line_number":1958,"context_line":"            result \u003d vref.conditional_update({\u0027status\u0027: \u0027reserved\u0027}, expected)"},{"line_number":1959,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"9f5c4f37_25e43bac","line":1956,"in_reply_to":"9f5c4f37_6597d392","updated":"2017-09-21 15:33:09.000000000","message":"That\u0027s kinda what I thought, but I\u0027m fine explicity fetching.  In this case though honestly why don\u0027t we just go back to using the DB directly?  Seems like it\u0027s a lot easier and more reliable.","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"35d6e6d1fdf361e66af173e1dd1738f5fd7bc30b","unresolved":false,"context_lines":[{"line_number":1953,"context_line":"        # in use.  We do this in Nova to address races in some of the"},{"line_number":1954,"context_line":"        # special cases like live-migration etc.  So to deal with"},{"line_number":1955,"context_line":"        # that we want to ignore in-use and reserved status here"},{"line_number":1956,"context_line":"        if vref.status not in [\u0027in-use\u0027, \u0027reserved\u0027]:"},{"line_number":1957,"context_line":"            expected \u003d {\u0027status\u0027: (\u0027available\u0027, \u0027downloading\u0027)}"},{"line_number":1958,"context_line":"            result \u003d vref.conditional_update({\u0027status\u0027: \u0027reserved\u0027}, expected)"},{"line_number":1959,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"9f5c4f37_6597d392","line":1956,"in_reply_to":"9f5c4f37_c588bf72","updated":"2017-09-21 14:17:31.000000000","message":"But wouldn\u0027t the conditional update protect against that?","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"a74b0a7c2b547364fbf05e0d952bb61003fe1f99","unresolved":false,"context_lines":[{"line_number":2055,"context_line":"        elif \u0027attaching\u0027 in pending_status_list:"},{"line_number":2056,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027attaching\u0027"},{"line_number":2057,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027attaching\u0027"},{"line_number":2058,"context_line":"        elif \u0027reserved\u0027 in pending_status_list:"},{"line_number":2059,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2060,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f5c4f37_2ecf7d80","line":2058,"range":{"start_line":2058,"start_character":14,"end_line":2058,"end_character":22},"updated":"2017-09-21 14:19:32.000000000","message":"I think \u0027reserved\u0027 should come after \u0027detaching\u0027 so we don\u0027t miss that state for the volume as we don\u0027t want two detaches happening at the same time for the first run at least.","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"63f7e80f8c45a5b4980f8ada110d86604781513a","unresolved":false,"context_lines":[{"line_number":2055,"context_line":"        elif \u0027attaching\u0027 in pending_status_list:"},{"line_number":2056,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027attaching\u0027"},{"line_number":2057,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027attaching\u0027"},{"line_number":2058,"context_line":"        elif \u0027reserved\u0027 in pending_status_list:"},{"line_number":2059,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2060,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"}],"source_content_type":"text/x-python","patch_set":5,"id":"7f515b1d_35e6baea","line":2058,"range":{"start_line":2058,"start_character":14,"end_line":2058,"end_character":22},"in_reply_to":"7f515b1d_8f2afd8d","updated":"2017-09-22 17:43:58.000000000","message":"Hmm, I thought we only do the lock if the uses_shared_connection flag is True, however I would still think we don\u0027t want to parallel detach operations happening on the same volume.\n\nIf the latter is not a concern or we will always have the lock, then ignore me plz. :)","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"d9286661262e7e382ec3e61e35c2f2aa3c1d3459","unresolved":false,"context_lines":[{"line_number":2055,"context_line":"        elif \u0027attaching\u0027 in pending_status_list:"},{"line_number":2056,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027attaching\u0027"},{"line_number":2057,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027attaching\u0027"},{"line_number":2058,"context_line":"        elif \u0027reserved\u0027 in pending_status_list:"},{"line_number":2059,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2060,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f5c4f37_96aafb82","line":2058,"range":{"start_line":2058,"start_character":14,"end_line":2058,"end_character":22},"in_reply_to":"9f5c4f37_05fff7aa","updated":"2017-09-21 19:39:08.000000000","message":"With the shared_connection case for multi-attach we will not be able to handle parallel detaches in my understanding.\n\nAnd why would the volume end up in \u0027available\u0027 as opposed to \u0027reserved\u0027?","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":2243,"name":"John Griffith","email":"john.griffith8@gmail.com","username":"john-griffith"},"change_message_id":"58b3cca8d77ca941590276b2b88cfe27b3966cf4","unresolved":false,"context_lines":[{"line_number":2055,"context_line":"        elif \u0027attaching\u0027 in pending_status_list:"},{"line_number":2056,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027attaching\u0027"},{"line_number":2057,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027attaching\u0027"},{"line_number":2058,"context_line":"        elif \u0027reserved\u0027 in pending_status_list:"},{"line_number":2059,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2060,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f5c4f37_05fff7aa","line":2058,"range":{"start_line":2058,"start_character":14,"end_line":2058,"end_character":22},"in_reply_to":"9f5c4f37_2ecf7d80","updated":"2017-09-21 15:33:09.000000000","message":"So then the volume is marked as avaialable and it\u0027s not, that\u0027s no good.  I\u0027m unclear why there\u0027s concern about multiple detaches?  Can you explain why that\u0027s a concern?","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":2243,"name":"John Griffith","email":"john.griffith8@gmail.com","username":"john-griffith"},"change_message_id":"ff234ec16e117ed367767eb3e27afe860c63207a","unresolved":false,"context_lines":[{"line_number":2055,"context_line":"        elif \u0027attaching\u0027 in pending_status_list:"},{"line_number":2056,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027attaching\u0027"},{"line_number":2057,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027attaching\u0027"},{"line_number":2058,"context_line":"        elif \u0027reserved\u0027 in pending_status_list:"},{"line_number":2059,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2060,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"}],"source_content_type":"text/x-python","patch_set":5,"id":"7f515b1d_8f2afd8d","line":2058,"range":{"start_line":2058,"start_character":14,"end_line":2058,"end_character":22},"in_reply_to":"9f5c4f37_96aafb82","updated":"2017-09-21 21:13:11.000000000","message":"Why not?  The locking mechanism to serialize it will be on the Nova side.  the volume would not go to reserved during a detach (unless there\u0027s a reserve in place already) because there is no \u0027reserve\u0027 state associated with attachment-delete (detaching).  The only path from in-use is \u0027available\u0027 which goes through detaching.  That\u0027s why this is checked first.  Make sure there are no attachment records associated with the volume that would prohibit us from saying \"available\"","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"a74b0a7c2b547364fbf05e0d952bb61003fe1f99","unresolved":false,"context_lines":[{"line_number":2058,"context_line":"        elif \u0027reserved\u0027 in pending_status_list:"},{"line_number":2059,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2060,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"},{"line_number":2062,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2063,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2064,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f5c4f37_e5d18359","line":2061,"range":{"start_line":2061,"start_character":13,"end_line":2061,"end_character":47},"updated":"2017-09-21 14:19:32.000000000","message":"The only issue here is that begin_detaching only sets the volume status to \u0027detaching\u0027: https://github.com/openstack/cinder/blob/master/cinder/volume/api.py#L701 Which means that the attachment will never be in this state so we will overwrite that volume state here never having this evaluate to True.","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"63f7e80f8c45a5b4980f8ada110d86604781513a","unresolved":false,"context_lines":[{"line_number":2058,"context_line":"        elif \u0027reserved\u0027 in pending_status_list:"},{"line_number":2059,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2060,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"},{"line_number":2062,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2063,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2064,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":5,"id":"7f515b1d_4d02b1b6","line":2061,"range":{"start_line":2061,"start_character":13,"end_line":2061,"end_character":47},"in_reply_to":"7f515b1d_1ad08d50","updated":"2017-09-22 17:43:58.000000000","message":"Yeah, I agree with your statements here. Just trying to ensure we don\u0027t shoot ourselves in the foot in the meantime with what we do in the new attach patch for the edge cases.","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":2243,"name":"John Griffith","email":"john.griffith8@gmail.com","username":"john-griffith"},"change_message_id":"ff234ec16e117ed367767eb3e27afe860c63207a","unresolved":false,"context_lines":[{"line_number":2058,"context_line":"        elif \u0027reserved\u0027 in pending_status_list:"},{"line_number":2059,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2060,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"},{"line_number":2062,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2063,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2064,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":5,"id":"7f515b1d_1ad08d50","line":2061,"range":{"start_line":2061,"start_character":13,"end_line":2061,"end_character":47},"in_reply_to":"7f515b1d_66624e19","updated":"2017-09-21 21:13:11.000000000","message":"Sure, but the point being that  this should be paired down into probably two things:  \"not-attached\" and \"attached\" (better terms would be good) but the point being that there\u0027s a flag that says \"you need to check attachment records for this volume, or you don\u0027t have to worry about it\".  But trying to maintain a state-machine here without building a state-machine is always going to be problematic.\n\nWhat I had initially thought was a coumn that said \"attachment-count \u003d x\" . Volume Status needs to be redefined and thought out again IMO, but I don\u0027t think that should happen here.","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"d9286661262e7e382ec3e61e35c2f2aa3c1d3459","unresolved":false,"context_lines":[{"line_number":2058,"context_line":"        elif \u0027reserved\u0027 in pending_status_list:"},{"line_number":2059,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2060,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"},{"line_number":2062,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2063,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2064,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":5,"id":"7f515b1d_66624e19","line":2061,"range":{"start_line":2061,"start_character":13,"end_line":2061,"end_character":47},"in_reply_to":"9f5c4f37_c50aff88","updated":"2017-09-21 19:39:08.000000000","message":"The issue with removing the checks is that the volume will end up in the wrong state and will potentially get stuck on the API layer. As we will have multiple attachments at the same time even before multi-attach it will be an issue early on for edge cases.","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":2243,"name":"John Griffith","email":"john.griffith8@gmail.com","username":"john-griffith"},"change_message_id":"58b3cca8d77ca941590276b2b88cfe27b3966cf4","unresolved":false,"context_lines":[{"line_number":2058,"context_line":"        elif \u0027reserved\u0027 in pending_status_list:"},{"line_number":2059,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2060,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027reserved\u0027"},{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"},{"line_number":2062,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2063,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2064,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f5c4f37_c50aff88","line":2061,"range":{"start_line":2061,"start_character":13,"end_line":2061,"end_character":47},"in_reply_to":"9f5c4f37_e5d18359","updated":"2017-09-21 15:33:09.000000000","message":"Very long sigh....\n\nThat\u0027s actually fine, so I removed the editorial and 20 line rant of comments regarding the 3 places we set status for attachment related things; but the TLDR is that it needs to be fixed.  It won\u0027t work for multi-attach at all, and it\u0027s terrible.  I can remove the check altogether if that\u0027s better here?  The fact is we shouldn\u0027t have all these different status values in 3 different places anyway.\n\nThere is a valid state in the attachment object for attaching/detaching etc, so it seems to me that since it\u0027s valid we should probably consider it, even if we don\u0027t seem to be using it currently.  I don\u0027t know... thoughts?","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"a74b0a7c2b547364fbf05e0d952bb61003fe1f99","unresolved":false,"context_lines":[{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"},{"line_number":2062,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2063,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2064,"context_line":"        else:"},{"line_number":2065,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027available\u0027"},{"line_number":2066,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027detached\u0027"},{"line_number":2067,"context_line":""},{"line_number":2068,"context_line":"        volume.status \u003d status_updates[\u0027status\u0027]"},{"line_number":2069,"context_line":"        volume.attach_status \u003d status_updates[\u0027attach_status\u0027]"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f5c4f37_4e8751d9","line":2066,"range":{"start_line":2064,"start_character":8,"end_line":2066,"end_character":56},"updated":"2017-09-21 14:19:32.000000000","message":"As you have these as default values the \u0027else\u0027 seems unnecessary here. Also I wonder whether we would want another if that checks that pending_status_list is not an empty list so we don\u0027t go through this all the time?","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":2243,"name":"John Griffith","email":"john.griffith8@gmail.com","username":"john-griffith"},"change_message_id":"58b3cca8d77ca941590276b2b88cfe27b3966cf4","unresolved":false,"context_lines":[{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"},{"line_number":2062,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2063,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2064,"context_line":"        else:"},{"line_number":2065,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027available\u0027"},{"line_number":2066,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027detached\u0027"},{"line_number":2067,"context_line":""},{"line_number":2068,"context_line":"        volume.status \u003d status_updates[\u0027status\u0027]"},{"line_number":2069,"context_line":"        volume.attach_status \u003d status_updates[\u0027attach_status\u0027]"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f5c4f37_e561833b","line":2066,"range":{"start_line":2064,"start_character":8,"end_line":2066,"end_character":56},"in_reply_to":"9f5c4f37_4e8751d9","updated":"2017-09-21 15:33:09.000000000","message":"I prefer to close the if/elif chain explicitly, if it makes people cringe then \"OK\" I\u0027ll remove the default settings above and just use an empty dict as the initializer.  \n\npending_status_list is built via the for loop of remaining attachments.  In other words if there are no other attachments there won\u0027t be anyhthing there so you won\u0027t needlessly iterate over it (it will be empty).  One enhancement I did consider is filtering out unique values, but honestly if this list is \u003e a few attachments I\u0027d be worried :)","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"d9286661262e7e382ec3e61e35c2f2aa3c1d3459","unresolved":false,"context_lines":[{"line_number":2061,"context_line":"        elif \u0027detaching\u0027 in pending_status_list:"},{"line_number":2062,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2063,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027detaching\u0027"},{"line_number":2064,"context_line":"        else:"},{"line_number":2065,"context_line":"            status_updates[\u0027status\u0027] \u003d \u0027available\u0027"},{"line_number":2066,"context_line":"            status_updates[\u0027attach_status\u0027] \u003d \u0027detached\u0027"},{"line_number":2067,"context_line":""},{"line_number":2068,"context_line":"        volume.status \u003d status_updates[\u0027status\u0027]"},{"line_number":2069,"context_line":"        volume.attach_status \u003d status_updates[\u0027attach_status\u0027]"}],"source_content_type":"text/x-python","patch_set":5,"id":"7f515b1d_46507233","line":2066,"range":{"start_line":2064,"start_character":8,"end_line":2066,"end_character":56},"in_reply_to":"9f5c4f37_e561833b","updated":"2017-09-21 19:39:08.000000000","message":"Fair enough, it\u0027s really not a concern, not even close. :)","commit_id":"62e3a1a376f626106fbb39ab523ddaf9e26b320c"}]}