)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"144520affeb0ec188072eca2f1bf6725c26c9389","unresolved":false,"context_lines":[{"line_number":15,"context_line":"because we are not yet setting the attachment_id when attaching"},{"line_number":16,"context_line":"volumes, that comes in a later change to the API."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"It\u0027s worth noting that when nova creates a volume during boot"},{"line_number":19,"context_line":"from volume for a source_type\u003dblank/image/snapshot BDM, we won\u0027t"},{"line_number":20,"context_line":"have an attachment_id for that volume so we\u0027ll attach using the"},{"line_number":21,"context_line":"old flow. For now this is OK, but might be something we need to"},{"line_number":22,"context_line":"revisit in the future when we eventually want to remove the old"},{"line_number":23,"context_line":"flow."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"The structure here is to separate the old/new attach flows into"},{"line_number":26,"context_line":"separate methods, despite quite a bit of copy/paste code in both"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"df87a7cf_3a6150d4","line":23,"range":{"start_line":18,"start_character":0,"end_line":23,"end_character":5},"updated":"2017-12-07 09:48:32.000000000","message":"I guess it does not effect booting from a volume that was pre-created in cinder and in that case nova will support multi attaching such volume.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c75985eb826216e7a612a51a42b03773dab33f00","unresolved":false,"context_lines":[{"line_number":15,"context_line":"because we are not yet setting the attachment_id when attaching"},{"line_number":16,"context_line":"volumes, that comes in a later change to the API."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"It\u0027s worth noting that when nova creates a volume during boot"},{"line_number":19,"context_line":"from volume for a source_type\u003dblank/image/snapshot BDM, we won\u0027t"},{"line_number":20,"context_line":"have an attachment_id for that volume so we\u0027ll attach using the"},{"line_number":21,"context_line":"old flow. For now this is OK, but might be something we need to"},{"line_number":22,"context_line":"revisit in the future when we eventually want to remove the old"},{"line_number":23,"context_line":"flow."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"The structure here is to separate the old/new attach flows into"},{"line_number":26,"context_line":"separate methods, despite quite a bit of copy/paste code in both"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"df87a7cf_7ff82148","line":23,"range":{"start_line":18,"start_character":0,"end_line":23,"end_character":5},"in_reply_to":"df87a7cf_291b48d0","updated":"2017-12-07 15:03:41.000000000","message":"When you boot from volume with a pre-existing volume (source_type\u003dvolume, destination_type\u003dvolume), the API will create the attachment to reserve the volume (next patch in this series). When you boot from volume without a pre-existing volume and source_type is blank/image/snapshot and destination_type\u003dvolume, nova creates the volume in cinder, but it does this in the compute service using the related DriverBlockDevice class for blank/image/snapshot. In that case nova has created the volume but doesn\u0027t immediately create an attachment for that volume, so the condition which checks to see if we should do legacy or new style attach in DriverVolumeBlockDevice.attach goes down the legacy path since we don\u0027t have an attachment. I\u0027m making a note of that here so we can do something about it later, but it doesn\u0027t break anything since we still handle old style attachments.\n\nI will add TODOs in the blank/image/snapshot driver classes to make this more clear.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"dcedd44d9752d02cb3be925345aa76777ad84042","unresolved":false,"context_lines":[{"line_number":15,"context_line":"because we are not yet setting the attachment_id when attaching"},{"line_number":16,"context_line":"volumes, that comes in a later change to the API."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"It\u0027s worth noting that when nova creates a volume during boot"},{"line_number":19,"context_line":"from volume for a source_type\u003dblank/image/snapshot BDM, we won\u0027t"},{"line_number":20,"context_line":"have an attachment_id for that volume so we\u0027ll attach using the"},{"line_number":21,"context_line":"old flow. For now this is OK, but might be something we need to"},{"line_number":22,"context_line":"revisit in the future when we eventually want to remove the old"},{"line_number":23,"context_line":"flow."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"The structure here is to separate the old/new attach flows into"},{"line_number":26,"context_line":"separate methods, despite quite a bit of copy/paste code in both"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"df87a7cf_291b48d0","line":23,"range":{"start_line":18,"start_character":0,"end_line":23,"end_character":5},"in_reply_to":"df87a7cf_3a6150d4","updated":"2017-12-07 13:02:09.000000000","message":"I got confused here. Using the old or new flow does not depend on who created the volume or at least it should not. It only depends on whether all the microversions are available when we call attachment_create to reserve the volume.\n\nAnd we do reserve the volume for the BFV case. The old reserve call was added when we removed check_attach and now we call attachment_create with the new Cinder flow. Is that not how it works in case Nova creates the volume? Am I missing something?\n\nWhat Nova doesn\u0027t set is the \u0027multiattach\u0027 flag, which will be False by default which I think is not an issue for now.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"144520affeb0ec188072eca2f1bf6725c26c9389","unresolved":false,"context_lines":[{"line_number":26,"context_line":"separate methods, despite quite a bit of copy/paste code in both"},{"line_number":27,"context_line":"places. A single method using conditionals in different places"},{"line_number":28,"context_line":"is arguably uglier (as seen in the test code) and we could likely"},{"line_number":29,"context_line":"consolidate some of the common code into helper methods later."},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"Note that test_refresh_connection_info_with_attachment_id is"},{"line_number":32,"context_line":"removed since it\u0027s now redundant with test_refresh_connection."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"df87a7cf_daa7040b","line":29,"updated":"2017-12-07 09:48:32.000000000","message":"I think that keeping a copy will help removing the old flow later without touching the code in the new flow making the removal safer","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"dcedd44d9752d02cb3be925345aa76777ad84042","unresolved":false,"context_lines":[{"line_number":26,"context_line":"separate methods, despite quite a bit of copy/paste code in both"},{"line_number":27,"context_line":"places. A single method using conditionals in different places"},{"line_number":28,"context_line":"is arguably uglier (as seen in the test code) and we could likely"},{"line_number":29,"context_line":"consolidate some of the common code into helper methods later."},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"Note that test_refresh_connection_info_with_attachment_id is"},{"line_number":32,"context_line":"removed since it\u0027s now redundant with test_refresh_connection."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"df87a7cf_a9267814","line":29,"in_reply_to":"df87a7cf_daa7040b","updated":"2017-12-07 13:02:09.000000000","message":"It was intentional in the later patch to copy the tests as opposed to consolidate so we have complete separation and an easier stage to handle removing the old flow.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"}],"nova/tests/unit/virt/test_block_device.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3c83898dd5be32ea9bfd771067016be1e5d754a5","unresolved":false,"context_lines":[{"line_number":661,"context_line":"        self.assertThat(test_bdm[\u0027connection_info\u0027],"},{"line_number":662,"context_line":"                        matchers.DictMatches(expected_conn_info))"},{"line_number":663,"context_line":""},{"line_number":664,"context_line":"    def test_refresh_connection_info_with_attachment_id(self):"},{"line_number":665,"context_line":"        \"\"\"Tests refreshing connection info when the DriverVolumeBlockDevice"},{"line_number":666,"context_line":"        has a new style attachment_id set, which is a call to attachment_get"},{"line_number":667,"context_line":"        rather than initialize_connection."}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_baab4a1b","side":"PARENT","line":664,"range":{"start_line":664,"start_character":8,"end_line":664,"end_character":55},"updated":"2017-12-05 23:54:12.000000000","message":"This is removed because it\u0027s already tested in test_refresh_connection.","commit_id":"4dd0656961552ed8418bb22352f8ed8f6f58031e"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"144520affeb0ec188072eca2f1bf6725c26c9389","unresolved":false,"context_lines":[{"line_number":226,"context_line":"        self.blank_bdm \u003d fake_block_device.fake_bdm_object("},{"line_number":227,"context_line":"            self.context, self.blank_bdm_dict)"},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"        # Set the attachment_id on our fake class variables which we have"},{"line_number":230,"context_line":"        # to do in setUp so that any attachment_id set by a subclass will"},{"line_number":231,"context_line":"        # be used properly."},{"line_number":232,"context_line":"        for name in (\u0027volume\u0027, \u0027snapshot\u0027, \u0027image\u0027, \u0027blank\u0027):"},{"line_number":233,"context_line":"            for attr in (\u0027%s_bdm\u0027, \u0027%s_driver_bdm\u0027):"},{"line_number":234,"context_line":"                bdm \u003d getattr(self, attr % name)"},{"line_number":235,"context_line":"                bdm[\u0027attachment_id\u0027] \u003d self.attachment_id"},{"line_number":236,"context_line":""},{"line_number":237,"context_line":"    def test_no_device_raises(self):"},{"line_number":238,"context_line":"        for name, cls in self.driver_classes.items():"}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_7a4cb841","line":235,"range":{"start_line":229,"start_character":0,"end_line":235,"end_character":57},"updated":"2017-12-07 09:48:32.000000000","message":"Does this make the above changes to each individual struct unnecessary? I removed those changes and the tests are still passing for me.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c75985eb826216e7a612a51a42b03773dab33f00","unresolved":false,"context_lines":[{"line_number":226,"context_line":"        self.blank_bdm \u003d fake_block_device.fake_bdm_object("},{"line_number":227,"context_line":"            self.context, self.blank_bdm_dict)"},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"        # Set the attachment_id on our fake class variables which we have"},{"line_number":230,"context_line":"        # to do in setUp so that any attachment_id set by a subclass will"},{"line_number":231,"context_line":"        # be used properly."},{"line_number":232,"context_line":"        for name in (\u0027volume\u0027, \u0027snapshot\u0027, \u0027image\u0027, \u0027blank\u0027):"},{"line_number":233,"context_line":"            for attr in (\u0027%s_bdm\u0027, \u0027%s_driver_bdm\u0027):"},{"line_number":234,"context_line":"                bdm \u003d getattr(self, attr % name)"},{"line_number":235,"context_line":"                bdm[\u0027attachment_id\u0027] \u003d self.attachment_id"},{"line_number":236,"context_line":""},{"line_number":237,"context_line":"    def test_no_device_raises(self):"},{"line_number":238,"context_line":"        for name, cls in self.driver_classes.items():"}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_bf7549cd","line":235,"range":{"start_line":229,"start_character":0,"end_line":235,"end_character":57},"in_reply_to":"df87a7cf_7a4cb841","updated":"2017-12-07 15:03:41.000000000","message":"The above changes are unnecessary yes, I debated whether or not to do that, but decided to leave it since it seemed weird to have an attachment class variable and not use it in the other class variables which have an attachment field. I can go either way.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"144520affeb0ec188072eca2f1bf6725c26c9389","unresolved":false,"context_lines":[{"line_number":772,"context_line":"        test_bdm \u003d self.driver_classes[\u0027snapshot\u0027]("},{"line_number":773,"context_line":"                fake_block_device.fake_bdm_object("},{"line_number":774,"context_line":"                        self.context, fail_volume_snapshot))"},{"line_number":775,"context_line":"        test_bdm.attachment_id \u003d self.attachment_id"},{"line_number":776,"context_line":""},{"line_number":777,"context_line":"        snapshot \u003d {\u0027id\u0027: \u0027fake-volume-id-1\u0027,"},{"line_number":778,"context_line":"                    \u0027attach_status\u0027: \u0027detached\u0027}"}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_7afb5878","line":775,"updated":"2017-12-07 09:48:32.000000000","message":"Isn\u0027t it also a case when nova creates the volume and therefore it uses the old flow?","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c75985eb826216e7a612a51a42b03773dab33f00","unresolved":false,"context_lines":[{"line_number":772,"context_line":"        test_bdm \u003d self.driver_classes[\u0027snapshot\u0027]("},{"line_number":773,"context_line":"                fake_block_device.fake_bdm_object("},{"line_number":774,"context_line":"                        self.context, fail_volume_snapshot))"},{"line_number":775,"context_line":"        test_bdm.attachment_id \u003d self.attachment_id"},{"line_number":776,"context_line":""},{"line_number":777,"context_line":"        snapshot \u003d {\u0027id\u0027: \u0027fake-volume-id-1\u0027,"},{"line_number":778,"context_line":"                    \u0027attach_status\u0027: \u0027detached\u0027}"}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_9f616508","line":775,"in_reply_to":"df87a7cf_7afb5878","updated":"2017-12-07 15:03:41.000000000","message":"Yeah I\u0027ll remove this.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"144520affeb0ec188072eca2f1bf6725c26c9389","unresolved":false,"context_lines":[{"line_number":784,"context_line":"        with test.nested("},{"line_number":785,"context_line":"            mock.patch.object(self.volume_api, \u0027get_snapshot\u0027,"},{"line_number":786,"context_line":"                              return_value\u003dsnapshot),"},{"line_number":787,"context_line":"            mock.patch.object(self.volume_api, \u0027create\u0027, return_value\u003dvolume),"},{"line_number":788,"context_line":"            mock.patch.object(self.volume_api, \u0027delete\u0027),"},{"line_number":789,"context_line":"        ) as (vol_get_snap, vol_create, vol_delete):"},{"line_number":790,"context_line":"            wait_func \u003d mock.MagicMock()"}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_7a4038a6","line":787,"updated":"2017-12-07 09:48:32.000000000","message":"This hints me that nova creates the volume","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c75985eb826216e7a612a51a42b03773dab33f00","unresolved":false,"context_lines":[{"line_number":784,"context_line":"        with test.nested("},{"line_number":785,"context_line":"            mock.patch.object(self.volume_api, \u0027get_snapshot\u0027,"},{"line_number":786,"context_line":"                              return_value\u003dsnapshot),"},{"line_number":787,"context_line":"            mock.patch.object(self.volume_api, \u0027create\u0027, return_value\u003dvolume),"},{"line_number":788,"context_line":"            mock.patch.object(self.volume_api, \u0027delete\u0027),"},{"line_number":789,"context_line":"        ) as (vol_get_snap, vol_create, vol_delete):"},{"line_number":790,"context_line":"            wait_func \u003d mock.MagicMock()"}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_ff48b187","line":787,"in_reply_to":"df87a7cf_7a4038a6","updated":"2017-12-07 15:03:41.000000000","message":"Yeah I think you\u0027re right, I was looking for a particular stub pattern on the volume_api.create call and missed this since it\u0027s using mock. It must not be a failure because the test fails in a different way, so it never gets to the code that cares about the attachment.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"144520affeb0ec188072eca2f1bf6725c26c9389","unresolved":false,"context_lines":[{"line_number":889,"context_line":"        test_bdm \u003d self.driver_classes[\u0027image\u0027]("},{"line_number":890,"context_line":"                fake_block_device.fake_bdm_object("},{"line_number":891,"context_line":"                        self.context, fail_volume_image))"},{"line_number":892,"context_line":"        test_bdm.attachment_id \u003d self.attachment_id"},{"line_number":893,"context_line":""},{"line_number":894,"context_line":"        image \u003d {\u0027id\u0027: \u0027fake-image-id-1\u0027}"},{"line_number":895,"context_line":"        volume \u003d {\u0027id\u0027: \u0027fake-volume-id-2\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_3a1f10b8","line":892,"updated":"2017-12-07 09:48:32.000000000","message":"Isn\u0027t it also a case when nova creates the volume and therefore it uses the old flow?","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c75985eb826216e7a612a51a42b03773dab33f00","unresolved":false,"context_lines":[{"line_number":889,"context_line":"        test_bdm \u003d self.driver_classes[\u0027image\u0027]("},{"line_number":890,"context_line":"                fake_block_device.fake_bdm_object("},{"line_number":891,"context_line":"                        self.context, fail_volume_image))"},{"line_number":892,"context_line":"        test_bdm.attachment_id \u003d self.attachment_id"},{"line_number":893,"context_line":""},{"line_number":894,"context_line":"        image \u003d {\u0027id\u0027: \u0027fake-image-id-1\u0027}"},{"line_number":895,"context_line":"        volume \u003d {\u0027id\u0027: \u0027fake-volume-id-2\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_bf2309bd","line":892,"in_reply_to":"df87a7cf_3a1f10b8","updated":"2017-12-07 15:03:41.000000000","message":"Yup same as above for snapshot.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c75985eb826216e7a612a51a42b03773dab33f00","unresolved":false,"context_lines":[{"line_number":940,"context_line":"                        self.virt_driver)"},{"line_number":941,"context_line":"        self.assertEqual(test_bdm.volume_id, \u0027fake-volume-id-2\u0027)"},{"line_number":942,"context_line":""},{"line_number":943,"context_line":"    def test_blank_attach_fail_volume(self):"},{"line_number":944,"context_line":"        no_blank_volume \u003d self.blank_bdm_dict.copy()"},{"line_number":945,"context_line":"        no_blank_volume[\u0027volume_id\u0027] \u003d None"},{"line_number":946,"context_line":"        test_bdm \u003d self.driver_classes[\u0027blank\u0027]("}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_9f2645ce","line":943,"range":{"start_line":943,"start_character":8,"end_line":943,"end_character":37},"updated":"2017-12-07 15:03:41.000000000","message":"Apparently I got this one correct. :)\n\nWe should consider adding a common private method for these 3 tests, but I\u0027d do that later (or never because I hate these tests).","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"a5b449139a956e52d521423c4f888e39567a479f","unresolved":false,"context_lines":[{"line_number":228,"context_line":"        for name in (\u0027volume\u0027, \u0027snapshot\u0027, \u0027image\u0027, \u0027blank\u0027):"},{"line_number":229,"context_line":"            for attr in (\u0027%s_bdm\u0027, \u0027%s_driver_bdm\u0027):"},{"line_number":230,"context_line":"                bdm \u003d getattr(self, attr % name)"},{"line_number":231,"context_line":"                bdm[\u0027attachment_id\u0027] \u003d self.attachment_id"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":"    def test_no_device_raises(self):"},{"line_number":234,"context_line":"        for name, cls in self.driver_classes.items():"}],"source_content_type":"text/x-python","patch_set":4,"id":"df87a7cf_8b74beff","line":231,"range":{"start_line":231,"start_character":16,"end_line":231,"end_character":57},"updated":"2017-12-07 20:01:25.000000000","message":"this will set bdm[\u0027attachment_id\u0027] to None instead of not having the attachment_id key exist in the bdm dict. Is that what you intended?","commit_id":"2d68fbe56582cebf4b0d8286e9821f863e452699"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a0c86b62c496ba564b091a85cc0e09d1a78fc7f6","unresolved":false,"context_lines":[{"line_number":228,"context_line":"        for name in (\u0027volume\u0027, \u0027snapshot\u0027, \u0027image\u0027, \u0027blank\u0027):"},{"line_number":229,"context_line":"            for attr in (\u0027%s_bdm\u0027, \u0027%s_driver_bdm\u0027):"},{"line_number":230,"context_line":"                bdm \u003d getattr(self, attr % name)"},{"line_number":231,"context_line":"                bdm[\u0027attachment_id\u0027] \u003d self.attachment_id"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":"    def test_no_device_raises(self):"},{"line_number":234,"context_line":"        for name, cls in self.driver_classes.items():"}],"source_content_type":"text/x-python","patch_set":4,"id":"df87a7cf_3e6292af","line":231,"range":{"start_line":231,"start_character":16,"end_line":231,"end_character":57},"in_reply_to":"df87a7cf_8b74beff","updated":"2017-12-07 20:04:55.000000000","message":"Well, this is needed for both test classes. The original one (old flow) hard-codes the attachment ID to None to avoid AttributeErrors in the DriverBlockDevice code. The new test needs this for setting the attachment_id to something that\u0027s not None to trigger the new flows. I agree it\u0027s ugly but that\u0027s kind of par for the course in these tests.","commit_id":"2d68fbe56582cebf4b0d8286e9821f863e452699"}],"nova/virt/block_device.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d6860842f5089bee921cb1e1a762110e76fb57f1","unresolved":false,"context_lines":[{"line_number":316,"context_line":"            # this connector is for the wrong host. Use the connector that"},{"line_number":317,"context_line":"            # was stored in connection_info instead (if we have one, and it"},{"line_number":318,"context_line":"            # is for the expected host)."},{"line_number":319,"context_line":"            stashed_connector \u003d connection_info.get(\u0027connector\u0027)"},{"line_number":320,"context_line":"            if not stashed_connector:"},{"line_number":321,"context_line":"                # Volume was attached before we began stashing connectors"},{"line_number":322,"context_line":"                LOG.warning(\"Host mismatch detected, but stashed \""}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_cf12f209","line":319,"updated":"2017-12-06 19:14:33.000000000","message":"For new-style attachments, we won\u0027t have a stashed connector in the bdm.connection_info because that only happens for initialize_connection with the old flow:\n\nhttps://github.com/openstack/nova/blob/master/nova/volume/cinder.py#L417\n\nCinder stores the host connector in the attachment_specs table, but there is no REST API to access that yet.\n\nThis could also likely mean that we aren\u0027t doing things with the new flow correctly during an evacuate.","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1ffc495ac806906de60f7985550080dc63958fe4","unresolved":false,"context_lines":[{"line_number":316,"context_line":"            # this connector is for the wrong host. Use the connector that"},{"line_number":317,"context_line":"            # was stored in connection_info instead (if we have one, and it"},{"line_number":318,"context_line":"            # is for the expected host)."},{"line_number":319,"context_line":"            stashed_connector \u003d connection_info.get(\u0027connector\u0027)"},{"line_number":320,"context_line":"            if not stashed_connector:"},{"line_number":321,"context_line":"                # Volume was attached before we began stashing connectors"},{"line_number":322,"context_line":"                LOG.warning(\"Host mismatch detected, but stashed \""}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_4fc5825a","line":319,"in_reply_to":"df87a7cf_cf12f209","updated":"2017-12-06 19:18:12.000000000","message":"Actually, in this case we shouldn\u0027t care about the connector because if we\u0027re doing the new flow properly, we don\u0027t need the connector since we don\u0027t use it below.","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1ffc495ac806906de60f7985550080dc63958fe4","unresolved":false,"context_lines":[{"line_number":347,"context_line":"                           \u0027schost\u0027: stashed_connector.get(\u0027host\u0027)})"},{"line_number":348,"context_line":"                connector \u003d stashed_connector"},{"line_number":349,"context_line":""},{"line_number":350,"context_line":"        # NOTE(jdg): For now we need to actually inspect the bdm for an"},{"line_number":351,"context_line":"        # attachment_id as opposed to relying on what may have been passed"},{"line_number":352,"context_line":"        # in, we want to force usage of the old detach flow for now and only"},{"line_number":353,"context_line":"        # use the new flow when we explicitly used it for the attach."},{"line_number":354,"context_line":"        if not self[\u0027attachment_id\u0027]:"},{"line_number":355,"context_line":"            volume_api.terminate_connection(context, volume_id, connector)"},{"line_number":356,"context_line":"            volume_api.detach(context.elevated(), volume_id, instance.uuid,"},{"line_number":357,"context_line":"                              attachment_id)"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_0fa53ab5","line":354,"range":{"start_line":350,"start_character":8,"end_line":354,"end_character":37},"updated":"2017-12-06 19:18:12.000000000","message":"Hmm, do we have something to change here? Can the stashed connector stuff above all slide under this conditional?","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"d4e05a11164d14b18ea9c6d6209445a89d774b7e","unresolved":false,"context_lines":[{"line_number":347,"context_line":"                           \u0027schost\u0027: stashed_connector.get(\u0027host\u0027)})"},{"line_number":348,"context_line":"                connector \u003d stashed_connector"},{"line_number":349,"context_line":""},{"line_number":350,"context_line":"        # NOTE(jdg): For now we need to actually inspect the bdm for an"},{"line_number":351,"context_line":"        # attachment_id as opposed to relying on what may have been passed"},{"line_number":352,"context_line":"        # in, we want to force usage of the old detach flow for now and only"},{"line_number":353,"context_line":"        # use the new flow when we explicitly used it for the attach."},{"line_number":354,"context_line":"        if not self[\u0027attachment_id\u0027]:"},{"line_number":355,"context_line":"            volume_api.terminate_connection(context, volume_id, connector)"},{"line_number":356,"context_line":"            volume_api.detach(context.elevated(), volume_id, instance.uuid,"},{"line_number":357,"context_line":"                              attachment_id)"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_e28a6d19","line":354,"range":{"start_line":350,"start_character":8,"end_line":354,"end_character":37},"in_reply_to":"df87a7cf_0fa53ab5","updated":"2017-12-06 20:00:54.000000000","message":"Yes we can. The rebuild call takes care of the attachments.","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3c83898dd5be32ea9bfd771067016be1e5d754a5","unresolved":false,"context_lines":[{"line_number":449,"context_line":"        self[\u0027connection_info\u0027] \u003d volume_api.attachment_update("},{"line_number":450,"context_line":"            context, attachment_id, connector)[\u0027connection_info\u0027]"},{"line_number":451,"context_line":""},{"line_number":452,"context_line":"        # # NOTE(ildikov): With the new attach flow Cinder stores the"},{"line_number":453,"context_line":"        # # connection_info as well. In principle we would like to retrieve the"},{"line_number":454,"context_line":"        # # information from Cinder, but it needs more refactoring in the code"},{"line_number":455,"context_line":"        # # so we keep storing it in the BDM for now."},{"line_number":456,"context_line":"        # connection_info_string \u003d jsonutils.dumps("},{"line_number":457,"context_line":"        #     self[\u0027connection_info\u0027])"},{"line_number":458,"context_line":"        # self._bdm_obj.connection_info \u003d connection_info_string"},{"line_number":459,"context_line":""},{"line_number":460,"context_line":"        if \u0027serial\u0027 not in self[\u0027connection_info\u0027]:"},{"line_number":461,"context_line":"            self[\u0027connection_info\u0027][\u0027serial\u0027] \u003d self.volume_id"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_bafd8a3b","line":458,"range":{"start_line":452,"start_character":0,"end_line":458,"end_character":64},"updated":"2017-12-05 23:54:12.000000000","message":"I intentionally commented this out, we can probably remove it - this is redundant with the save() and @update_db calls.","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"5e58bda8abf2b120ddc3658664a0d3531c159b14","unresolved":false,"context_lines":[{"line_number":449,"context_line":"        self[\u0027connection_info\u0027] \u003d volume_api.attachment_update("},{"line_number":450,"context_line":"            context, attachment_id, connector)[\u0027connection_info\u0027]"},{"line_number":451,"context_line":""},{"line_number":452,"context_line":"        # # NOTE(ildikov): With the new attach flow Cinder stores the"},{"line_number":453,"context_line":"        # # connection_info as well. In principle we would like to retrieve the"},{"line_number":454,"context_line":"        # # information from Cinder, but it needs more refactoring in the code"},{"line_number":455,"context_line":"        # # so we keep storing it in the BDM for now."},{"line_number":456,"context_line":"        # connection_info_string \u003d jsonutils.dumps("},{"line_number":457,"context_line":"        #     self[\u0027connection_info\u0027])"},{"line_number":458,"context_line":"        # self._bdm_obj.connection_info \u003d connection_info_string"},{"line_number":459,"context_line":""},{"line_number":460,"context_line":"        if \u0027serial\u0027 not in self[\u0027connection_info\u0027]:"},{"line_number":461,"context_line":"            self[\u0027connection_info\u0027][\u0027serial\u0027] \u003d self.volume_id"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_c39f3801","line":458,"range":{"start_line":452,"start_character":0,"end_line":458,"end_character":64},"in_reply_to":"df87a7cf_bafd8a3b","updated":"2017-12-06 14:24:54.000000000","message":"Ah, ok. I admit I had no idea that it might be covered by any of those, my knowledge seems pretty much non-existent regarding this part. Thanks for catching it!","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"053feb25269c0548209165e01a0c3443d68b7e67","unresolved":false,"context_lines":[{"line_number":449,"context_line":"        self[\u0027connection_info\u0027] \u003d volume_api.attachment_update("},{"line_number":450,"context_line":"            context, attachment_id, connector)[\u0027connection_info\u0027]"},{"line_number":451,"context_line":""},{"line_number":452,"context_line":"        # # NOTE(ildikov): With the new attach flow Cinder stores the"},{"line_number":453,"context_line":"        # # connection_info as well. In principle we would like to retrieve the"},{"line_number":454,"context_line":"        # # information from Cinder, but it needs more refactoring in the code"},{"line_number":455,"context_line":"        # # so we keep storing it in the BDM for now."},{"line_number":456,"context_line":"        # connection_info_string \u003d jsonutils.dumps("},{"line_number":457,"context_line":"        #     self[\u0027connection_info\u0027])"},{"line_number":458,"context_line":"        # self._bdm_obj.connection_info \u003d connection_info_string"},{"line_number":459,"context_line":""},{"line_number":460,"context_line":"        if \u0027serial\u0027 not in self[\u0027connection_info\u0027]:"},{"line_number":461,"context_line":"            self[\u0027connection_info\u0027][\u0027serial\u0027] \u003d self.volume_id"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_f28fa420","line":458,"range":{"start_line":452,"start_character":0,"end_line":458,"end_character":64},"in_reply_to":"df87a7cf_c39f3801","updated":"2017-12-06 16:58:55.000000000","message":"Not a problem, this object code is very confusing and overly tricky for it\u0027s own good. It wraps a BlockDeviceMapping object and passes through the save using it\u0027s own save() method, and the @update_db decorator. Very easy to forget where and when things happen. I only caught this because I\u0027ve dealt with some nasty bugs in this code depending on where and when (or don\u0027t) save the updates.\n\nIb95c8f7b66aca0c4ac7b92d140cbeb5e85c2717f","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"b9b62c5ad2bfb67b5988a710845437a7c63abe13","unresolved":false,"context_lines":[{"line_number":449,"context_line":"        self[\u0027connection_info\u0027] \u003d volume_api.attachment_update("},{"line_number":450,"context_line":"            context, attachment_id, connector)[\u0027connection_info\u0027]"},{"line_number":451,"context_line":""},{"line_number":452,"context_line":"        # # NOTE(ildikov): With the new attach flow Cinder stores the"},{"line_number":453,"context_line":"        # # connection_info as well. In principle we would like to retrieve the"},{"line_number":454,"context_line":"        # # information from Cinder, but it needs more refactoring in the code"},{"line_number":455,"context_line":"        # # so we keep storing it in the BDM for now."},{"line_number":456,"context_line":"        # connection_info_string \u003d jsonutils.dumps("},{"line_number":457,"context_line":"        #     self[\u0027connection_info\u0027])"},{"line_number":458,"context_line":"        # self._bdm_obj.connection_info \u003d connection_info_string"},{"line_number":459,"context_line":""},{"line_number":460,"context_line":"        if \u0027serial\u0027 not in self[\u0027connection_info\u0027]:"},{"line_number":461,"context_line":"            self[\u0027connection_info\u0027][\u0027serial\u0027] \u003d self.volume_id"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_3d363de8","line":458,"range":{"start_line":452,"start_character":0,"end_line":458,"end_character":64},"in_reply_to":"df87a7cf_f28fa420","updated":"2017-12-06 17:14:53.000000000","message":"Cool, that makes me feel a little better now. Thanks. :)","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3c83898dd5be32ea9bfd771067016be1e5d754a5","unresolved":false,"context_lines":[{"line_number":464,"context_line":"            self.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":465,"context_line":""},{"line_number":466,"context_line":"        self._preserve_multipath_id(self[\u0027connection_info\u0027])"},{"line_number":467,"context_line":"        # FIXME(mriedem): Why don\u0027t we do this *after* the"},{"line_number":468,"context_line":"        # virt_driver.attach_volume call is successful like in the legacy"},{"line_number":469,"context_line":"        # flow? Otherwise we now have a BDM with connection_info set in Nova"},{"line_number":470,"context_line":"        # even though we failed to connect the volume."},{"line_number":471,"context_line":"        self.save()"},{"line_number":472,"context_line":"        if do_driver_attach:"},{"line_number":473,"context_line":"            encryption \u003d encryptors.get_encryption_metadata("},{"line_number":474,"context_line":"                context, volume_api, volume_id, self[\u0027connection_info\u0027])"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_daf88e4a","line":471,"range":{"start_line":467,"start_character":8,"end_line":471,"end_character":19},"updated":"2017-12-05 23:54:12.000000000","message":"Open question.","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"053feb25269c0548209165e01a0c3443d68b7e67","unresolved":false,"context_lines":[{"line_number":464,"context_line":"            self.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":465,"context_line":""},{"line_number":466,"context_line":"        self._preserve_multipath_id(self[\u0027connection_info\u0027])"},{"line_number":467,"context_line":"        # FIXME(mriedem): Why don\u0027t we do this *after* the"},{"line_number":468,"context_line":"        # virt_driver.attach_volume call is successful like in the legacy"},{"line_number":469,"context_line":"        # flow? Otherwise we now have a BDM with connection_info set in Nova"},{"line_number":470,"context_line":"        # even though we failed to connect the volume."},{"line_number":471,"context_line":"        self.save()"},{"line_number":472,"context_line":"        if do_driver_attach:"},{"line_number":473,"context_line":"            encryption \u003d encryptors.get_encryption_metadata("},{"line_number":474,"context_line":"                context, volume_api, volume_id, self[\u0027connection_info\u0027])"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_f22b24e8","line":471,"range":{"start_line":467,"start_character":8,"end_line":471,"end_character":19},"in_reply_to":"df87a7cf_231864d1","updated":"2017-12-06 16:58:55.000000000","message":"OK. So looking back on this bug fix:\n\nhttps://review.openstack.org/#/c/164330/2/nova/virt/block_device.py\n\nWe definitely need to save the connection_info updates after self[\u0027connection_info\u0027] is set and before attachment_complete is set (which sets the volume status to in-use).\n\nAt this point we\u0027ve done that.\n\nThe issue is, are we setting self[\u0027connection_info\u0027] and saving it too early? In the old flow, we only do this *after* we\u0027ve connected the host via the driver (which goes through os-brick connect_volume).\n\nLooking at how the old code works, we pass the local connection_info variable through to virt_driver.attach_volume and once that is successful, we save the connection_info in the BDM object that this DriverBDM object wraps.\n\nSo I think that\u0027s probably what we want to do here too to be consistent.","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"5e58bda8abf2b120ddc3658664a0d3531c159b14","unresolved":false,"context_lines":[{"line_number":464,"context_line":"            self.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":465,"context_line":""},{"line_number":466,"context_line":"        self._preserve_multipath_id(self[\u0027connection_info\u0027])"},{"line_number":467,"context_line":"        # FIXME(mriedem): Why don\u0027t we do this *after* the"},{"line_number":468,"context_line":"        # virt_driver.attach_volume call is successful like in the legacy"},{"line_number":469,"context_line":"        # flow? Otherwise we now have a BDM with connection_info set in Nova"},{"line_number":470,"context_line":"        # even though we failed to connect the volume."},{"line_number":471,"context_line":"        self.save()"},{"line_number":472,"context_line":"        if do_driver_attach:"},{"line_number":473,"context_line":"            encryption \u003d encryptors.get_encryption_metadata("},{"line_number":474,"context_line":"                context, volume_api, volume_id, self[\u0027connection_info\u0027])"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_231864d1","line":471,"range":{"start_line":467,"start_character":8,"end_line":471,"end_character":19},"in_reply_to":"df87a7cf_daf88e4a","updated":"2017-12-06 14:24:54.000000000","message":"The very easy answer to this question is that we had one here and one at the end of this function. I removed the second after John Garbutt\u0027s comment here: https://review.openstack.org/#/c/330285/153/nova/virt/block_device.py@492","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"b9b62c5ad2bfb67b5988a710845437a7c63abe13","unresolved":false,"context_lines":[{"line_number":464,"context_line":"            self.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":465,"context_line":""},{"line_number":466,"context_line":"        self._preserve_multipath_id(self[\u0027connection_info\u0027])"},{"line_number":467,"context_line":"        # FIXME(mriedem): Why don\u0027t we do this *after* the"},{"line_number":468,"context_line":"        # virt_driver.attach_volume call is successful like in the legacy"},{"line_number":469,"context_line":"        # flow? Otherwise we now have a BDM with connection_info set in Nova"},{"line_number":470,"context_line":"        # even though we failed to connect the volume."},{"line_number":471,"context_line":"        self.save()"},{"line_number":472,"context_line":"        if do_driver_attach:"},{"line_number":473,"context_line":"            encryption \u003d encryptors.get_encryption_metadata("},{"line_number":474,"context_line":"                context, volume_api, volume_id, self[\u0027connection_info\u0027])"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_9d6509e4","line":471,"range":{"start_line":467,"start_character":8,"end_line":471,"end_character":19},"in_reply_to":"df87a7cf_f22b24e8","updated":"2017-12-06 17:14:53.000000000","message":"Yeah, that makes sense as we might need to roll back and then we have the connection_info set for no good reason.","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3c83898dd5be32ea9bfd771067016be1e5d754a5","unresolved":false,"context_lines":[{"line_number":487,"context_line":"                                  instance\u003dinstance)"},{"line_number":488,"context_line":"                    volume_api.attachment_delete(context,"},{"line_number":489,"context_line":"                                                 attachment_id)"},{"line_number":490,"context_line":"        # TODO(mriedem): Do we need to handle a failure here and call"},{"line_number":491,"context_line":"        # attachment_delete and/or driver_detach if do_driver_attach is True?"},{"line_number":492,"context_line":"        # That\u0027s what the legacy flow does."},{"line_number":493,"context_line":"        volume_api.attachment_complete(context, attachment_id)"},{"line_number":494,"context_line":""},{"line_number":495,"context_line":"    @update_db"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_9a98c61b","line":492,"range":{"start_line":490,"start_character":8,"end_line":492,"end_character":43},"updated":"2017-12-05 23:54:12.000000000","message":"Open question.","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"b9b62c5ad2bfb67b5988a710845437a7c63abe13","unresolved":false,"context_lines":[{"line_number":487,"context_line":"                                  instance\u003dinstance)"},{"line_number":488,"context_line":"                    volume_api.attachment_delete(context,"},{"line_number":489,"context_line":"                                                 attachment_id)"},{"line_number":490,"context_line":"        # TODO(mriedem): Do we need to handle a failure here and call"},{"line_number":491,"context_line":"        # attachment_delete and/or driver_detach if do_driver_attach is True?"},{"line_number":492,"context_line":"        # That\u0027s what the legacy flow does."},{"line_number":493,"context_line":"        volume_api.attachment_complete(context, attachment_id)"},{"line_number":494,"context_line":""},{"line_number":495,"context_line":"    @update_db"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_7d9845bf","line":492,"range":{"start_line":490,"start_character":8,"end_line":492,"end_character":43},"in_reply_to":"df87a7cf_32774c05","updated":"2017-12-06 17:14:53.000000000","message":"Yes. Sorry if it wasn\u0027t 100% clear from the previous comment.","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"5e58bda8abf2b120ddc3658664a0d3531c159b14","unresolved":false,"context_lines":[{"line_number":487,"context_line":"                                  instance\u003dinstance)"},{"line_number":488,"context_line":"                    volume_api.attachment_delete(context,"},{"line_number":489,"context_line":"                                                 attachment_id)"},{"line_number":490,"context_line":"        # TODO(mriedem): Do we need to handle a failure here and call"},{"line_number":491,"context_line":"        # attachment_delete and/or driver_detach if do_driver_attach is True?"},{"line_number":492,"context_line":"        # That\u0027s what the legacy flow does."},{"line_number":493,"context_line":"        volume_api.attachment_complete(context, attachment_id)"},{"line_number":494,"context_line":""},{"line_number":495,"context_line":"    @update_db"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_c35c1880","line":492,"range":{"start_line":490,"start_character":8,"end_line":492,"end_character":43},"in_reply_to":"df87a7cf_9a98c61b","updated":"2017-12-06 14:24:54.000000000","message":"Just by thinking about the situation it would nice to just retry as by this point the volume is actually connected and it\u0027s final state change that might not happened as of yet. But as I don\u0027t think we\u0027re going to make our lives miserable on this one, due to the fact that the volume will be in \u0027attaching\u0027 state I think we will need to do error handling on this one.\n\nOriginally this call was called in the try-except block after vrit_driver.attach_volume and got moved here when we said that it should always happen in this call as opposed to being all over the place.","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"053feb25269c0548209165e01a0c3443d68b7e67","unresolved":false,"context_lines":[{"line_number":487,"context_line":"                                  instance\u003dinstance)"},{"line_number":488,"context_line":"                    volume_api.attachment_delete(context,"},{"line_number":489,"context_line":"                                                 attachment_id)"},{"line_number":490,"context_line":"        # TODO(mriedem): Do we need to handle a failure here and call"},{"line_number":491,"context_line":"        # attachment_delete and/or driver_detach if do_driver_attach is True?"},{"line_number":492,"context_line":"        # That\u0027s what the legacy flow does."},{"line_number":493,"context_line":"        volume_api.attachment_complete(context, attachment_id)"},{"line_number":494,"context_line":""},{"line_number":495,"context_line":"    @update_db"}],"source_content_type":"text/x-python","patch_set":1,"id":"df87a7cf_32774c05","line":492,"range":{"start_line":490,"start_character":8,"end_line":492,"end_character":43},"in_reply_to":"df87a7cf_c35c1880","updated":"2017-12-06 16:58:55.000000000","message":"So I think you\u0027re saying yes we should handle any errors and delete the attachment so the volume is back to \u0027available\u0027, just like in the old flow. Agree?","commit_id":"56964b0f2c8dc5b3218f363ccb3422c34eaf75c3"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"144520affeb0ec188072eca2f1bf6725c26c9389","unresolved":false,"context_lines":[{"line_number":445,"context_line":"        # details like instance.id and connection_info.  Only trick"},{"line_number":446,"context_line":"        # is if we do \"multi-attach to same instance\""},{"line_number":447,"context_line":"        # We do this special handling for things like unshelve/migrate"},{"line_number":448,"context_line":"        connector[\u0027mount_device\u0027] \u003d self[\u0027mount_device\u0027]"},{"line_number":449,"context_line":"        LOG.debug(\"Updating existing attachment record: %s\","},{"line_number":450,"context_line":"                  attachment_id)"},{"line_number":451,"context_line":"        connection_info \u003d volume_api.attachment_update("}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_3dc38a3a","line":448,"range":{"start_line":448,"start_character":0,"end_line":448,"end_character":56},"updated":"2017-12-07 09:48:32.000000000","message":"This seems to be a difference between the old and the new flow. But I failed to see the reason of this. The unit and functional tests passing without this line.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c75985eb826216e7a612a51a42b03773dab33f00","unresolved":false,"context_lines":[{"line_number":445,"context_line":"        # details like instance.id and connection_info.  Only trick"},{"line_number":446,"context_line":"        # is if we do \"multi-attach to same instance\""},{"line_number":447,"context_line":"        # We do this special handling for things like unshelve/migrate"},{"line_number":448,"context_line":"        connector[\u0027mount_device\u0027] \u003d self[\u0027mount_device\u0027]"},{"line_number":449,"context_line":"        LOG.debug(\"Updating existing attachment record: %s\","},{"line_number":450,"context_line":"                  attachment_id)"},{"line_number":451,"context_line":"        connection_info \u003d volume_api.attachment_update("}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_e206d61c","line":448,"range":{"start_line":448,"start_character":0,"end_line":448,"end_character":56},"in_reply_to":"df87a7cf_3dc38a3a","updated":"2017-12-07 15:03:41.000000000","message":"Hmm, yeah, I\u0027m not sure. I can ask John Griffith who wrote this. We should remove it if it\u0027s not used.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a4a0174c29f0c30433975cbc821e86c03e82c487","unresolved":false,"context_lines":[{"line_number":445,"context_line":"        # details like instance.id and connection_info.  Only trick"},{"line_number":446,"context_line":"        # is if we do \"multi-attach to same instance\""},{"line_number":447,"context_line":"        # We do this special handling for things like unshelve/migrate"},{"line_number":448,"context_line":"        connector[\u0027mount_device\u0027] \u003d self[\u0027mount_device\u0027]"},{"line_number":449,"context_line":"        LOG.debug(\"Updating existing attachment record: %s\","},{"line_number":450,"context_line":"                  attachment_id)"},{"line_number":451,"context_line":"        connection_info \u003d volume_api.attachment_update("}],"source_content_type":"text/x-python","patch_set":3,"id":"df87a7cf_358491d2","line":448,"range":{"start_line":448,"start_character":0,"end_line":448,"end_character":56},"in_reply_to":"df87a7cf_e206d61c","updated":"2017-12-12 15:57:18.000000000","message":"Apparently this is needed, see bug 1737779.","commit_id":"b53ae6b10ac253a178d030d09da05bdcca6e46a1"}]}