)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":17130,"name":"melissaml","email":"ma.lei@99cloud.net","username":"malei"},"change_message_id":"a45914ba9199892bca1435b6cfd9e0f6c7a1052f","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Fix issue with not removing rbd rescue disk"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Currently when instance that use RBD as backend"},{"line_number":10,"context_line":"is rescued and next unrescued, rescue image is"},{"line_number":11,"context_line":"not removed, this cause issue when the same"},{"line_number":12,"context_line":"instance is rescued again it\u0027s use old rescue"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"9a89bdaa_f6da54e7","line":9,"range":{"start_line":9,"start_character":29,"end_line":9,"end_character":32},"updated":"2016-09-07 09:23:39.000000000","message":"uses","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":17130,"name":"melissaml","email":"ma.lei@99cloud.net","username":"malei"},"change_message_id":"a45914ba9199892bca1435b6cfd9e0f6c7a1052f","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Fix issue with not removing rbd rescue disk"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Currently when instance that use RBD as backend"},{"line_number":10,"context_line":"is rescued and next unrescued, rescue image is"},{"line_number":11,"context_line":"not removed, this cause issue when the same"},{"line_number":12,"context_line":"instance is rescued again it\u0027s use old rescue"},{"line_number":13,"context_line":"image not new one."},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"9a89bdaa_0afcecb6","line":11,"range":{"start_line":10,"start_character":44,"end_line":11,"end_character":11},"updated":"2016-09-07 09:23:39.000000000","message":"can not be removed","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":17130,"name":"melissaml","email":"ma.lei@99cloud.net","username":"malei"},"change_message_id":"a45914ba9199892bca1435b6cfd9e0f6c7a1052f","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Currently when instance that use RBD as backend"},{"line_number":10,"context_line":"is rescued and next unrescued, rescue image is"},{"line_number":11,"context_line":"not removed, this cause issue when the same"},{"line_number":12,"context_line":"instance is rescued again it\u0027s use old rescue"},{"line_number":13,"context_line":"image not new one."},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"9a89bdaa_3654bc1f","line":11,"range":{"start_line":11,"start_character":24,"end_line":11,"end_character":29},"updated":"2016-09-07 09:23:39.000000000","message":"the issue","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":17130,"name":"melissaml","email":"ma.lei@99cloud.net","username":"malei"},"change_message_id":"a45914ba9199892bca1435b6cfd9e0f6c7a1052f","unresolved":false,"context_lines":[{"line_number":9,"context_line":"Currently when instance that use RBD as backend"},{"line_number":10,"context_line":"is rescued and next unrescued, rescue image is"},{"line_number":11,"context_line":"not removed, this cause issue when the same"},{"line_number":12,"context_line":"instance is rescued again it\u0027s use old rescue"},{"line_number":13,"context_line":"image not new one."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: Idf4086303baa4b936c90be89552ad8deb45cef3a"},{"line_number":16,"context_line":"Closes-Bug: #1475652"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"9a89bdaa_8a9d5cbe","line":13,"range":{"start_line":12,"start_character":26,"end_line":13,"end_character":18},"updated":"2016-09-07 09:23:39.000000000","message":"it will use the old rescue image but not the new one.","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"21988c76a253dfb2d108572de0d147f23e9b0ba2","unresolved":false,"context_lines":[{"line_number":9,"context_line":"Currently when instance that use RBD as backend"},{"line_number":10,"context_line":"is rescued and next unrescued, rescue image is"},{"line_number":11,"context_line":"not removed, this cause issue when the same"},{"line_number":12,"context_line":"instance is rescued again it\u0027s use old rescue"},{"line_number":13,"context_line":"image not new one."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: Idf4086303baa4b936c90be89552ad8deb45cef3a"},{"line_number":16,"context_line":"Closes-Bug: #1475652"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"9a89bdaa_ad65dab5","line":13,"range":{"start_line":12,"start_character":26,"end_line":13,"end_character":18},"in_reply_to":"9a89bdaa_8a9d5cbe","updated":"2016-09-07 09:39:48.000000000","message":"Hi, these are all good comments, but in general they would be considered \u0027nits\u0027. i.e. Things which could be fixed, but aren\u0027t very important. Your -1 says: Nova is better without this patch, which is not true of these comments. Without expressing any other reason you should not, in general, -1 for things like this. For the person on the receiving end it feels like a \u0027drive-by\u0027 review, and they might suspect it\u0027s done purely to game stackalytics. Receiving a lot of low-value negative reviews can be demoralising, which is bad for the health of the community.\n\nmelwitt has spotted an actual bug and an associated test deficiency, which is obviously a good reason to use a -1. If you\u0027ve also checked melwitt\u0027s review and agree with it, you should say that in your own review when leaving a -1.","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"}],"nova/tests/unit/virt/libvirt/storage/test_rbd.py":[{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"ec029e644854d332849712a189e04d275ddf3858","unresolved":false,"context_lines":[{"line_number":341,"context_line":"    @mock.patch.object(rbd_utils, \u0027rados\u0027)"},{"line_number":342,"context_line":"    @mock.patch.object(rbd_utils, \u0027RADOSClient\u0027)"},{"line_number":343,"context_line":"    def test_cleanup_rescue_volumes(self, mock_client, mock_rados, mock_rbd):"},{"line_number":344,"context_line":"        instance \u003d objects.Instance(id\u003d1, uuid\u003d\u002712345\u0027, tast_state\u003dNone)"},{"line_number":345,"context_line":""},{"line_number":346,"context_line":"        rbd \u003d mock_rbd.RBD.return_value"},{"line_number":347,"context_line":"        rbd.list.return_value \u003d [\u002712345_test\u0027, \u0027111_test\u0027,"}],"source_content_type":"text/x-python","patch_set":5,"id":"9abb7d3a_24ca486f","line":344,"updated":"2016-06-02 09:48:50.000000000","message":"nit, there\u0027s a typo in the above line, tast_state should be task_state.","commit_id":"23cb5324d9debfa5771bfa21d9f9de5d4ad1eaca"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"07c861ddc96d7f63d6ed34828c65c194a79d083c","unresolved":false,"context_lines":[{"line_number":327,"context_line":"    @mock.patch.object(rbd_utils, \u0027RADOSClient\u0027)"},{"line_number":328,"context_line":"    def test_cleanup_volumes(self, mock_client, mock_rados, mock_rbd):"},{"line_number":329,"context_line":"        instance \u003d objects.Instance(id\u003d1, uuid\u003d\u002712345\u0027, task_state\u003dNone)"},{"line_number":330,"context_line":"        filter_fn \u003d lambda disk: disk.startswith(instance.uuid)"},{"line_number":331,"context_line":""},{"line_number":332,"context_line":"        rbd \u003d mock_rbd.RBD.return_value"},{"line_number":333,"context_line":"        rbd.list.return_value \u003d [\u002712345_test\u0027, \u0027111_test\u0027]"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_2d4e8296","line":330,"updated":"2016-06-09 22:49:40.000000000","message":"This is a problem with using lambdas, there isn\u0027t any connection to the implementation calling cleanup_volumes. You are testing that the right thing happens when you pass this filter function, but nothing is testing that the right filter functions are being used in driver.py\n\nI suggest defining them as global functions in rbd_utils.py and using them instead of lambdas","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"2dbfc8d1ae4501f1a41e53363aa25a37730f76aa","unresolved":false,"context_lines":[{"line_number":327,"context_line":"    @mock.patch.object(rbd_utils, \u0027RADOSClient\u0027)"},{"line_number":328,"context_line":"    def test_cleanup_volumes(self, mock_client, mock_rados, mock_rbd):"},{"line_number":329,"context_line":"        instance \u003d objects.Instance(id\u003d1, uuid\u003d\u002712345\u0027, task_state\u003dNone)"},{"line_number":330,"context_line":"        filter_fn \u003d lambda disk: disk.startswith(instance.uuid)"},{"line_number":331,"context_line":""},{"line_number":332,"context_line":"        rbd \u003d mock_rbd.RBD.return_value"},{"line_number":333,"context_line":"        rbd.list.return_value \u003d [\u002712345_test\u0027, \u0027111_test\u0027]"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_e3dc8648","line":330,"in_reply_to":"7aa08908_2d4e8296","updated":"2016-06-10 00:58:32.000000000","message":"Correction: global functions would go in driver.py since the point of the refactor was the remove awareness of the disk naming scheme from rbd_utils","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"c004a324aa7d6e1d317d79dfb84518d4ea50f4b5","unresolved":false,"context_lines":[{"line_number":327,"context_line":"    @mock.patch.object(rbd_utils, \u0027RADOSClient\u0027)"},{"line_number":328,"context_line":"    def test_cleanup_volumes(self, mock_client, mock_rados, mock_rbd):"},{"line_number":329,"context_line":"        instance \u003d objects.Instance(id\u003d1, uuid\u003d\u002712345\u0027, task_state\u003dNone)"},{"line_number":330,"context_line":"        filter_fn \u003d lambda disk: disk.startswith(instance.uuid)"},{"line_number":331,"context_line":""},{"line_number":332,"context_line":"        rbd \u003d mock_rbd.RBD.return_value"},{"line_number":333,"context_line":"        rbd.list.return_value \u003d [\u002712345_test\u0027, \u0027111_test\u0027]"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_eafd4b3c","line":330,"in_reply_to":"7aa08908_54a89677","updated":"2016-06-10 19:12:14.000000000","message":"Yeah, I saw your point about disk naming scheme not really belonging in rbd_utils but since the format was already leaked over there, I was okay with the .rescue being there too. There was actually another patch before this [1] that took a slightly different approach of passing the \u0027.rescue\u0027 suffix in, thereby not introducing more format awareness in to rbd_utils.\n\n[1] https://review.openstack.org/#/c/241777/","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4d0f7ef899f20188774e21baef7d46d1e0791bae","unresolved":false,"context_lines":[{"line_number":327,"context_line":"    @mock.patch.object(rbd_utils, \u0027RADOSClient\u0027)"},{"line_number":328,"context_line":"    def test_cleanup_volumes(self, mock_client, mock_rados, mock_rbd):"},{"line_number":329,"context_line":"        instance \u003d objects.Instance(id\u003d1, uuid\u003d\u002712345\u0027, task_state\u003dNone)"},{"line_number":330,"context_line":"        filter_fn \u003d lambda disk: disk.startswith(instance.uuid)"},{"line_number":331,"context_line":""},{"line_number":332,"context_line":"        rbd \u003d mock_rbd.RBD.return_value"},{"line_number":333,"context_line":"        rbd.list.return_value \u003d [\u002712345_test\u0027, \u0027111_test\u0027]"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_54a89677","line":330,"in_reply_to":"7aa08908_e3dc8648","updated":"2016-06-10 14:07:27.000000000","message":"I made this point in an earlier version of the patch. Ironically I thought he was more likely to get pushback for *including* it :) Glad to be wrong.","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"07c861ddc96d7f63d6ed34828c65c194a79d083c","unresolved":false,"context_lines":[{"line_number":333,"context_line":"        rbd.list.return_value \u003d [\u002712345_test\u0027, \u0027111_test\u0027]"},{"line_number":334,"context_line":""},{"line_number":335,"context_line":"        client \u003d mock_client.return_value"},{"line_number":336,"context_line":"        self.driver.cleanup_volumes(filter_fn)"},{"line_number":337,"context_line":"        rbd.remove.assert_called_once_with(client.ioctx, \u002712345_test\u0027)"},{"line_number":338,"context_line":"        client.__enter__.assert_called_once_with()"},{"line_number":339,"context_line":"        client.__exit__.assert_called_once_with(None, None, None)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_2851500f","line":336,"updated":"2016-06-09 22:49:40.000000000","message":"For example, this might be:\n\n self.driver.cleanup_volumes(rbd_utils.default_filter)\n\nif you had say, default_filter, local_filter, and rescue_filter global functions","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4d0f7ef899f20188774e21baef7d46d1e0791bae","unresolved":false,"context_lines":[{"line_number":333,"context_line":"        rbd.list.return_value \u003d [\u002712345_test\u0027, \u0027111_test\u0027]"},{"line_number":334,"context_line":""},{"line_number":335,"context_line":"        client \u003d mock_client.return_value"},{"line_number":336,"context_line":"        self.driver.cleanup_volumes(filter_fn)"},{"line_number":337,"context_line":"        rbd.remove.assert_called_once_with(client.ioctx, \u002712345_test\u0027)"},{"line_number":338,"context_line":"        client.__enter__.assert_called_once_with()"},{"line_number":339,"context_line":"        client.__exit__.assert_called_once_with(None, None, None)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_547c16b5","line":336,"in_reply_to":"7aa08908_2851500f","updated":"2016-06-10 14:07:27.000000000","message":"Problem is, the filter function genuinely is a closure around instance. This would have to be driver.default_filter(instance), where default_filter returned a lambda. The only way to see if the filter does the correct thing is to execute it.\n\nIn my view, the most useful way to test this would be to mock out the relevant bits of rbd_utils, actually execute cleanup_volumes from the driver with the passed in filter, and determine that the correct volumes were deleted. This would be testing driver and rbd_utils in concert, though, and we seem to have an ideological aversion to this.","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"c004a324aa7d6e1d317d79dfb84518d4ea50f4b5","unresolved":false,"context_lines":[{"line_number":333,"context_line":"        rbd.list.return_value \u003d [\u002712345_test\u0027, \u0027111_test\u0027]"},{"line_number":334,"context_line":""},{"line_number":335,"context_line":"        client \u003d mock_client.return_value"},{"line_number":336,"context_line":"        self.driver.cleanup_volumes(filter_fn)"},{"line_number":337,"context_line":"        rbd.remove.assert_called_once_with(client.ioctx, \u002712345_test\u0027)"},{"line_number":338,"context_line":"        client.__enter__.assert_called_once_with()"},{"line_number":339,"context_line":"        client.__exit__.assert_called_once_with(None, None, None)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_cdb069e0","line":336,"in_reply_to":"7aa08908_547c16b5","updated":"2016-06-10 19:12:14.000000000","message":"Ah, right. The instance complicates it. I agree the approach you describe seems the only way to properly test what we have here.\n\nI think it\u0027s more a practical aversion. From a maintainability perspective, it\u0027s simple to know as a general rule that test_driver.py tests the implementation of functions in driver.py and mocks anything lower, and repeat that pattern. When we have something like this, we have to know that in this case, we shouldn\u0027t mock cleanup_volumes and let it run for real, otherwise we miss testing the implementation. Over time, I think it\u0027s not as easy to keep track of those cases and is more subject to regression than others that are more traditional. Scenario: 6 months from now a developer thinks to do some cleanup \u0027mock cleanup_volumes in test_driver.py and avoid unnecessary overhead\u0027","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"07c861ddc96d7f63d6ed34828c65c194a79d083c","unresolved":false,"context_lines":[{"line_number":415,"context_line":"                                           \u002712345_test_disk.local\u0027)"},{"line_number":416,"context_line":"        client.__enter__.assert_called_once_with()"},{"line_number":417,"context_line":"        client.__exit__.assert_called_once_with(None, None, None)"},{"line_number":418,"context_line":""},{"line_number":419,"context_line":"    @mock.patch.object(rbd_utils, \u0027rbd\u0027)"},{"line_number":420,"context_line":"    @mock.patch.object(rbd_utils, \u0027rados\u0027)"},{"line_number":421,"context_line":"    @mock.patch.object(rbd_utils, \u0027RADOSClient\u0027)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_e8c0e82c","line":418,"updated":"2016-06-09 22:49:40.000000000","message":"I think you also need to add a test for the .rescue case","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"94291f5bdac624e238f42b0d40132049dbc31e1c","unresolved":false,"context_lines":[{"line_number":340,"context_line":"    @mock.patch.object(rbd_utils, \u0027RADOSClient\u0027)"},{"line_number":341,"context_line":"    def test_cleanup_volumes(self, mock_client, mock_rados, mock_rbd):"},{"line_number":342,"context_line":"        instance \u003d objects.Instance(id\u003d1, uuid\u003duuids.instance, task_state\u003dNone)"},{"line_number":343,"context_line":"        filter_fn \u003d lambda disk: disk.startswith(instance.uuid)"},{"line_number":344,"context_line":""},{"line_number":345,"context_line":"        rbd \u003d mock_rbd.RBD.return_value"},{"line_number":346,"context_line":"        rbd.list.return_value \u003d [\u0027%s_test\u0027 % uuids.instance, \u0027111_test\u0027]"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a89bdaa_de565929","line":343,"range":{"start_line":343,"start_character":8,"end_line":343,"end_character":63},"updated":"2016-09-07 02:22:19.000000000","message":"Please add a code comment that this is duplicated from nova/virt/libvirt/driver.py","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"94291f5bdac624e238f42b0d40132049dbc31e1c","unresolved":false,"context_lines":[{"line_number":359,"context_line":"    def _test_cleanup_exception(self, exception_name,"},{"line_number":360,"context_line":"                                mock_client, mock_rados, mock_rbd):"},{"line_number":361,"context_line":"        instance \u003d objects.Instance(id\u003d1, uuid\u003duuids.instance, task_state\u003dNone)"},{"line_number":362,"context_line":"        filter_fn \u003d lambda disk: disk.startswith(instance.uuid)"},{"line_number":363,"context_line":""},{"line_number":364,"context_line":"        setattr(mock_rbd, exception_name, test.TestingException)"},{"line_number":365,"context_line":"        rbd \u003d mock_rbd.RBD.return_value"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a89bdaa_de2d79a9","line":362,"range":{"start_line":362,"start_character":8,"end_line":362,"end_character":63},"updated":"2016-09-07 02:22:19.000000000","message":"Please add a code comment that this is duplicated from nova/virt/libvirt/driver.py","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"94291f5bdac624e238f42b0d40132049dbc31e1c","unresolved":false,"context_lines":[{"line_number":390,"context_line":"    def test_cleanup_volumes_pending_resize(self, mock_proxy, mock_client,"},{"line_number":391,"context_line":"                                            mock_rados, mock_rbd):"},{"line_number":392,"context_line":"        instance \u003d objects.Instance(id\u003d1, uuid\u003duuids.instance, task_state\u003dNone)"},{"line_number":393,"context_line":"        filter_fn \u003d lambda disk: disk.startswith(instance.uuid)"},{"line_number":394,"context_line":""},{"line_number":395,"context_line":"        setattr(mock_rbd, \u0027ImageHasSnapshots\u0027, test.TestingException)"},{"line_number":396,"context_line":"        rbd \u003d mock_rbd.RBD.return_value"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a89bdaa_09a6cd52","line":393,"range":{"start_line":393,"start_character":8,"end_line":393,"end_character":63},"updated":"2016-09-07 02:22:19.000000000","message":"Please add a code comment that this is duplicated from nova/virt/libvirt/driver.py","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"94291f5bdac624e238f42b0d40132049dbc31e1c","unresolved":false,"context_lines":[{"line_number":417,"context_line":"                                              mock_rbd):"},{"line_number":418,"context_line":"        instance \u003d objects.Instance(id\u003d1, uuid\u003duuids.instance,"},{"line_number":419,"context_line":"                                    task_state\u003dtask_states.RESIZE_REVERTING)"},{"line_number":420,"context_line":"        filter_fn \u003d lambda disk: (disk.startswith(instance.uuid) and"},{"line_number":421,"context_line":"                                  disk.endswith(\u0027disk.local\u0027))"},{"line_number":422,"context_line":""},{"line_number":423,"context_line":"        rbd \u003d mock_rbd.RBD.return_value"},{"line_number":424,"context_line":"        rbd.list.return_value \u003d [\u0027%s_test\u0027 % uuids.instance, \u0027111_test\u0027,"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a89bdaa_a97961ca","line":421,"range":{"start_line":420,"start_character":8,"end_line":421,"end_character":62},"updated":"2016-09-07 02:22:19.000000000","message":"Please add a code comment that this is duplicated from nova/virt/libvirt/driver.py","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ff97d7b0992b615a6b525f3021064bb5bb58644e","unresolved":false,"context_lines":[{"line_number":339,"context_line":"    @mock.patch.object(rbd_utils, \u0027rados\u0027)"},{"line_number":340,"context_line":"    @mock.patch.object(rbd_utils, \u0027RADOSClient\u0027)"},{"line_number":341,"context_line":"    def test_cleanup_volumes(self, mock_client, mock_rados, mock_rbd):"},{"line_number":342,"context_line":"        instance \u003d objects.Instance(id\u003d1, uuid\u003duuids.instance, task_state\u003dNone)"},{"line_number":343,"context_line":"        # this is duplicated from nova/virt/libvirt/driver.py"},{"line_number":344,"context_line":"        filter_fn \u003d lambda disk: disk.startswith(instance.uuid)"},{"line_number":345,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"fa6399be_176e1d01","line":342,"updated":"2016-10-12 13:56:57.000000000","message":"AFAICT, this is unrelated to the patch.","commit_id":"8695ed466dd48e3447d03c773387fb5661aef6c7"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ff97d7b0992b615a6b525f3021064bb5bb58644e","unresolved":false,"context_lines":[{"line_number":359,"context_line":"    @mock.patch.object(rbd_utils, \u0027RADOSClient\u0027)"},{"line_number":360,"context_line":"    def _test_cleanup_exception(self, exception_name,"},{"line_number":361,"context_line":"                                mock_client, mock_rados, mock_rbd):"},{"line_number":362,"context_line":"        instance \u003d objects.Instance(id\u003d1, uuid\u003duuids.instance, task_state\u003dNone)"},{"line_number":363,"context_line":"        # this is duplicated from nova/virt/libvirt/driver.py"},{"line_number":364,"context_line":"        filter_fn \u003d lambda disk: disk.startswith(instance.uuid)"},{"line_number":365,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"fa6399be_b77d71b3","line":362,"updated":"2016-10-12 13:56:57.000000000","message":"This too?","commit_id":"8695ed466dd48e3447d03c773387fb5661aef6c7"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"225018c4161d79e0b43cd73996b42fcdb21267f1","unresolved":false,"context_lines":[{"line_number":15110,"context_line":"    @mock.patch.object(libvirt_utils, \u0027get_instance_path\u0027)"},{"line_number":15111,"context_line":"    @mock.patch.object(libvirt_utils, \u0027load_file\u0027)"},{"line_number":15112,"context_line":"    @mock.patch.object(host.Host, \"get_domain\")"},{"line_number":15113,"context_line":"    def test_unrescue_remove_rbd(self, mock_get_domain, mock_load_file,"},{"line_number":15114,"context_line":"                                                      mock_get_instance_path):"},{"line_number":15115,"context_line":"        dummyxml \u003d (\"\u003cdomain type\u003d\u0027kvm\u0027\u003e\u003cname\u003einstance-0000000b\u003c/name\u003e\""},{"line_number":15116,"context_line":"                    \"\u003cdevices\u003e\""}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_5bade686","line":15113,"updated":"2016-05-11 09:43:29.000000000","message":"maybe better name would be \"test_unrescue_rbd\" and then change also name of \"test_unrescue\" in L15069 to something like \"test_unrescue_lvm\"?\nOr maybe even move all common code from those two tests to one helper method (\"_test_unrescue\") and then call it with different parameters in those two tests?","commit_id":"8acb5fec0d21f4634bed99a3d373c7de554b9ec9"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"48976f1117c24535370377a517e097c7fc49cbfe","unresolved":false,"context_lines":[{"line_number":15149,"context_line":"                      mock_remove_rbd, mock_glob):"},{"line_number":15150,"context_line":""},{"line_number":15151,"context_line":"            drvr.unrescue(instance, None)"},{"line_number":15152,"context_line":"            mock_write.assert_called_once_with(xml_path, \"fake_unrescue_xml\")"},{"line_number":15153,"context_line":"            mock_destroy.assert_called_once_with(instance)"},{"line_number":15154,"context_line":"            mock_create.assert_called_once_with(\"fake_unrescue_xml\","},{"line_number":15155,"context_line":"                                                 fake_dom)"},{"line_number":15156,"context_line":"            self.assertEqual(2, mock_del.call_count)"},{"line_number":15157,"context_line":"            self.assertEqual(unrescue_xml_path,"},{"line_number":15158,"context_line":"                             mock_del.call_args_list[0][0][0])"},{"line_number":15159,"context_line":"            self.assertEqual(rescue_file, mock_del.call_args_list[1][0][0])"},{"line_number":15160,"context_line":"            mock_remove_volumes.assert_called_once_with([])"},{"line_number":15161,"context_line":"            mock_get_rbd.assert_called_once_with()"},{"line_number":15162,"context_line":"            mock_remove_rbd.assert_called_once_with(\"fake\u003duuid_disk.rescue\")"},{"line_number":15163,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"dab17558_4a84d3d1","line":15160,"range":{"start_line":15152,"start_character":12,"end_line":15160,"end_character":59},"updated":"2016-05-17 15:45:34.000000000","message":"This code does not really help with testing changes related to RBD and is already covered by test_unrescue.\n\nYou can also avoid duplicated code by adding private method with all test code. Before calling private method you can mock RBD related methods, set CONF.libvirt.images_type to RBD and then validate mock.","commit_id":"8acb5fec0d21f4634bed99a3d373c7de554b9ec9"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"9b6952911c0f5c1aff1cc8637eade9f47c3a83af","unresolved":false,"context_lines":[{"line_number":15107,"context_line":"            mock_remove_volumes.assert_called_once_with([\u0027lvm.rescue\u0027])"},{"line_number":15108,"context_line":""},{"line_number":15109,"context_line":"    def test_unrescue(self):"},{"line_number":15110,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)"},{"line_number":15111,"context_line":"        self._test_unrescue(drvr\u003ddrvr)"},{"line_number":15112,"context_line":""},{"line_number":15113,"context_line":"    def test_unrescue_rbd(self):"}],"source_content_type":"text/x-python","patch_set":2,"id":"bab6814e_d50caa44","line":15110,"updated":"2016-05-18 14:06:12.000000000","message":"IMO this drvr is not necessary here. It can be moved to _test_unrescue method","commit_id":"cef3df164bdf71419ae76bab3910e86c4385f9d0"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"9b6952911c0f5c1aff1cc8637eade9f47c3a83af","unresolved":false,"context_lines":[{"line_number":15111,"context_line":"        self._test_unrescue(drvr\u003ddrvr)"},{"line_number":15112,"context_line":""},{"line_number":15113,"context_line":"    def test_unrescue_rbd(self):"},{"line_number":15114,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)"},{"line_number":15115,"context_line":"        CONF.set_override(\u0027images_type\u0027, \u0027rbd\u0027, \u0027libvirt\u0027)"},{"line_number":15116,"context_line":"        with test.nested("},{"line_number":15117,"context_line":"            mock.patch.object(drvr, \u0027_get_rbd_driver\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"bab6814e_1507521d","line":15114,"updated":"2016-05-18 14:06:12.000000000","message":"ditto","commit_id":"cef3df164bdf71419ae76bab3910e86c4385f9d0"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"b272b4f3e8ad2a3874bf9f126c529d8935fad963","unresolved":false,"context_lines":[{"line_number":10532,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)"},{"line_number":10533,"context_line":"        drvr._cleanup_rbd(instance)"},{"line_number":10534,"context_line":""},{"line_number":10535,"context_line":"        driver.cleanup_volumes.assert_called_once_with(mock.ANY)"},{"line_number":10536,"context_line":""},{"line_number":10537,"context_line":"    @mock.patch.object(objects.Instance, \u0027save\u0027)"},{"line_number":10538,"context_line":"    def test_destroy_undefines_no_undefine_flags(self, mock_save):"}],"source_content_type":"text/x-python","patch_set":6,"id":"7aa08908_260f3189","line":10535,"updated":"2016-06-08 11:09:06.000000000","message":"My personal preference would be to unit test both _cleanup_rbd and cleanup_volumes here, and to assert that the filter function we pass in works as expected with a given set of existing volumes. However, that\u0027s not consistent with my experience of other Openstack unit tests, so unfortunately I think this is the best we\u0027re going to get.\n\nIOW, I\u0027m reluctantly ok with this.","commit_id":"6b39d02dfb15d9e1c55769104f0f8dabf3f6432c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"07c861ddc96d7f63d6ed34828c65c194a79d083c","unresolved":false,"context_lines":[{"line_number":10532,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)"},{"line_number":10533,"context_line":"        drvr._cleanup_rbd(instance)"},{"line_number":10534,"context_line":""},{"line_number":10535,"context_line":"        driver.cleanup_volumes.assert_called_once_with(mock.ANY)"},{"line_number":10536,"context_line":""},{"line_number":10537,"context_line":"    @mock.patch.object(objects.Instance, \u0027save\u0027)"},{"line_number":10538,"context_line":"    def test_destroy_undefines_no_undefine_flags(self, mock_save):"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_ad6432e5","line":10535,"updated":"2016-06-09 22:49:40.000000000","message":"This isn\u0027t testing that _cleanup_rbd does the right thing because nothing is verifying that a correct filter function is being passed to cleanup_volumes. And the cleanup_volumes tests are setting the filter functions themselves, thus not testing the implementation in driver.py. If you change to global functions as mentioned in the other file, you can assert on the filter function that should have been passed.","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"91db86e9c66c2855aa95fd212fe1e62537e994e5","unresolved":false,"context_lines":[{"line_number":10532,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)"},{"line_number":10533,"context_line":"        drvr._cleanup_rbd(instance)"},{"line_number":10534,"context_line":""},{"line_number":10535,"context_line":"        driver.cleanup_volumes.assert_called_once_with(mock.ANY)"},{"line_number":10536,"context_line":""},{"line_number":10537,"context_line":"    @mock.patch.object(objects.Instance, \u0027save\u0027)"},{"line_number":10538,"context_line":"    def test_destroy_undefines_no_undefine_flags(self, mock_save):"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_b748865d","line":10535,"in_reply_to":"7aa08908_31e5c6a3","updated":"2016-06-13 17:25:27.000000000","message":"I\u0027d rather not fake cleanup_volumes as we would end up effectively duplicating cleanup_volumes implementation over here, and these tests would think things are working if the cleanup_volumes implementation/tests ever changes.\n\nI think we can just go ahead and test it as a unit here, let it call all the way down to cleanup_volumes and verify the result. We should have code comments in this test explaining why it\u0027s done and that should prevent potential issues with maintenance down the road.\n\nSo I think here we would use the real RBDDriver and mock its _destroy_volume and verify the volume arg of those calls. With that, I think test_cleanup_volumes_reverting_resize in test_rbd.py should be removed and tested in test_driver.py instead as a new test \u0027test_cleanup_rbd_reverting_resize\u0027, and the .rescue test case would go in test_driver.py as part of the existing test_unrescue. The other cleanup_volumes tests in test_rbd.py can stay as they are because they\u0027re testing the context manager and the flow in the case of exceptions. The filter function specifics could probably be removed from them if we test all of that here.","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"57a16ce65a9681ef8fa8ddfc72b53dc43b41caf5","unresolved":false,"context_lines":[{"line_number":10532,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)"},{"line_number":10533,"context_line":"        drvr._cleanup_rbd(instance)"},{"line_number":10534,"context_line":""},{"line_number":10535,"context_line":"        driver.cleanup_volumes.assert_called_once_with(mock.ANY)"},{"line_number":10536,"context_line":""},{"line_number":10537,"context_line":"    @mock.patch.object(objects.Instance, \u0027save\u0027)"},{"line_number":10538,"context_line":"    def test_destroy_undefines_no_undefine_flags(self, mock_save):"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_31e5c6a3","line":10535,"in_reply_to":"7aa08908_ad6432e5","updated":"2016-06-13 16:26:59.000000000","message":"Was thinking about this earlier. If we faked cleanup_volumes() here, we could execute the given filter function in the fake against a known input, and we wouldn\u0027t have the cross-module issues we\u0027re fretting about.","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"07c861ddc96d7f63d6ed34828c65c194a79d083c","unresolved":false,"context_lines":[{"line_number":10533,"context_line":"        drvr._cleanup_rbd(instance)"},{"line_number":10534,"context_line":""},{"line_number":10535,"context_line":"        driver.cleanup_volumes.assert_called_once_with(mock.ANY)"},{"line_number":10536,"context_line":""},{"line_number":10537,"context_line":"    @mock.patch.object(objects.Instance, \u0027save\u0027)"},{"line_number":10538,"context_line":"    def test_destroy_undefines_no_undefine_flags(self, mock_save):"},{"line_number":10539,"context_line":"        mock \u003d self.mox.CreateMock(fakelibvirt.virDomain)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_482adce8","line":10536,"updated":"2016-06-09 22:49:40.000000000","message":"Since the logic moved from cleanup_volumes to cleanup_rbd, I think you need another test in here to verify the right filter function is passed to cleanup_volumes when task_state is RESIZE_REVERTING","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"08bf294714d3e826bab444307ba0731db01831ea","unresolved":false,"context_lines":[{"line_number":11806,"context_line":"    @mock.patch.object(rbd_utils, \u0027rbd\u0027)"},{"line_number":11807,"context_line":"    @mock.patch.object(rbd_utils, \u0027rados\u0027)"},{"line_number":11808,"context_line":"    def test_cleanup_rbd(self, mock_rados, mock_rbd, mock_destroy_volume):"},{"line_number":11809,"context_line":"        driver \u003d rbd_utils.RBDDriver(\u0027xaxa\u0027, \u0027/path/bar.conf\u0027, None)"},{"line_number":11810,"context_line":"        mock_destroy_volume.return_value \u003d None"},{"line_number":11811,"context_line":""},{"line_number":11812,"context_line":"        instance \u003d objects.Instance(**self.test_instance)"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_afdd0472","line":11809,"updated":"2016-08-04 12:20:46.000000000","message":"This isn\u0027t necessary, and kinda confusing. The object you create here is never used. The assert you put on it down below happens to work because you mocked the class above, and all instances share the same mock.","commit_id":"c3a8c769162708f39b3fd9c67d8ab7343c5d00b8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"08bf294714d3e826bab444307ba0731db01831ea","unresolved":false,"context_lines":[{"line_number":11810,"context_line":"        mock_destroy_volume.return_value \u003d None"},{"line_number":11811,"context_line":""},{"line_number":11812,"context_line":"        instance \u003d objects.Instance(**self.test_instance)"},{"line_number":11813,"context_line":"        rbd_name \u003d instance.uuid + \u0027_\u0027 + instance.name"},{"line_number":11814,"context_line":"        all_volumes \u003d [str(uuid.uuid4()) + \u0027_disk\u0027,"},{"line_number":11815,"context_line":"                       str(uuid.uuid4()) + \u0027_disk.swap\u0027,"},{"line_number":11816,"context_line":"                       rbd_name"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_8f0e4030","line":11813,"updated":"2016-08-04 12:20:46.000000000","message":"This isn\u0027t quite right. From imagebackend.Rbd.__init__:\n\n  self.rbd_name \u003d \u0027%s_%s\u0027 % (instance.uuid, disk_name)\n\nYou want to put a couple of disks here to be deleted.","commit_id":"c3a8c769162708f39b3fd9c67d8ab7343c5d00b8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"08bf294714d3e826bab444307ba0731db01831ea","unresolved":false,"context_lines":[{"line_number":11811,"context_line":""},{"line_number":11812,"context_line":"        instance \u003d objects.Instance(**self.test_instance)"},{"line_number":11813,"context_line":"        rbd_name \u003d instance.uuid + \u0027_\u0027 + instance.name"},{"line_number":11814,"context_line":"        all_volumes \u003d [str(uuid.uuid4()) + \u0027_disk\u0027,"},{"line_number":11815,"context_line":"                       str(uuid.uuid4()) + \u0027_disk.swap\u0027,"},{"line_number":11816,"context_line":"                       rbd_name"},{"line_number":11817,"context_line":"                      ]"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_0fbd30d7","line":11814,"updated":"2016-08-04 12:20:46.000000000","message":"Have a look at the uuidsentinel module, imported in this class as \u0027uuids\u0027. If you just use \u0027uuids.other_instance\u0027 here, that will automatically be created and be a valid uuid.","commit_id":"c3a8c769162708f39b3fd9c67d8ab7343c5d00b8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"08bf294714d3e826bab444307ba0731db01831ea","unresolved":false,"context_lines":[{"line_number":11814,"context_line":"        all_volumes \u003d [str(uuid.uuid4()) + \u0027_disk\u0027,"},{"line_number":11815,"context_line":"                       str(uuid.uuid4()) + \u0027_disk.swap\u0027,"},{"line_number":11816,"context_line":"                       rbd_name"},{"line_number":11817,"context_line":"                      ]"},{"line_number":11818,"context_line":""},{"line_number":11819,"context_line":"        mock_rbd.RBD.return_value.list.return_value \u003d all_volumes"},{"line_number":11820,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_6f994c6e","line":11817,"updated":"2016-08-04 12:20:46.000000000","message":"How about something like:\n[\n  uuids.other_instance + \u0027_disk\u0027,\n  uuids.other_instance + \u0027_disk.swap\u0027,\n  instance.uuid + \u0027_disk\u0027,\n  instance.uuid + \u0027_disk.swap\u0027\n]\n\nThen below you\u0027d want to assert that just the last 2 were deleted.","commit_id":"c3a8c769162708f39b3fd9c67d8ab7343c5d00b8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"08bf294714d3e826bab444307ba0731db01831ea","unresolved":false,"context_lines":[{"line_number":11820,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)"},{"line_number":11821,"context_line":"        drvr._cleanup_rbd(instance)"},{"line_number":11822,"context_line":""},{"line_number":11823,"context_line":"        driver._destroy_volume.assert_called_once_with(mock.ANY, rbd_name)"},{"line_number":11824,"context_line":""},{"line_number":11825,"context_line":"    @mock.patch.object(rbd_utils, \u0027rbd\u0027)"},{"line_number":11826,"context_line":"    @mock.patch.object(rbd_utils, \u0027rados\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_af72247a","line":11823,"updated":"2016-08-04 12:20:46.000000000","message":"You can just assert on the class\u0027s mock here. No need to use an otherwise unused object.","commit_id":"c3a8c769162708f39b3fd9c67d8ab7343c5d00b8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"08bf294714d3e826bab444307ba0731db01831ea","unresolved":false,"context_lines":[{"line_number":11839,"context_line":"                                 \u0027%s_test_disk.local\u0027 % uuids.instance]"},{"line_number":11840,"context_line":""},{"line_number":11841,"context_line":"        client \u003d mock_client.return_value"},{"line_number":11842,"context_line":"        driver.cleanup_volumes(filter_fn)"},{"line_number":11843,"context_line":"        rbd.remove.assert_called_once_with(mock.ANY,"},{"line_number":11844,"context_line":"                                           \u0027%s_test_disk.local\u0027 % uuids.instance)"},{"line_number":11845,"context_line":"        client.__enter__.assert_called_once_with()"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_4f38c83d","line":11842,"updated":"2016-08-04 12:20:46.000000000","message":"This is a test of RBDDriver. It doesn\u0027t live here. Did you move the other test here?","commit_id":"c3a8c769162708f39b3fd9c67d8ab7343c5d00b8"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"9629fa4700640f6280b436c293ec7d342122b7ca","unresolved":false,"context_lines":[{"line_number":11819,"context_line":"        drvr._cleanup_rbd(instance)"},{"line_number":11820,"context_line":"        calls \u003d [mock.call(mock.ANY, instance.uuid + \u0027_disk\u0027),"},{"line_number":11821,"context_line":"                 mock.call(mock.ANY, instance.uuid + \u0027_disk.swap\u0027)]"},{"line_number":11822,"context_line":"        mock_destroy_volume.assert_has_calls(calls)"},{"line_number":11823,"context_line":""},{"line_number":11824,"context_line":"    @mock.patch.object(objects.Instance, \u0027save\u0027)"},{"line_number":11825,"context_line":"    def test_destroy_undefines_no_undefine_flags(self, mock_save):"}],"source_content_type":"text/x-python","patch_set":13,"id":"fa7ab95a_42bdb9d6","line":11822,"updated":"2016-08-30 13:06:08.000000000","message":"Unfortunately this doesn\u0027t fully assert what we\u0027re trying to test here, which is that:\n\n1. instance_disk and instance_disk.swap are deleted\n2. other_instance_disk and other_instance_disk.swap are NOT deleted\n\nYou assert 1, but not 2. Specifically, if you updated filter_fn in _cleanup_rbd to be \"lambda disk: True\", this test would still pass.\n\nIf you additionally assert mock_destroy_volume.call_count \u003d\u003d 2 that would cover it. There may also be a better way.","commit_id":"ea3b4db9c7d0e026529831e03ed10cd1dfc78a9f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"94291f5bdac624e238f42b0d40132049dbc31e1c","unresolved":false,"context_lines":[{"line_number":11806,"context_line":"    @mock.patch.object(rbd_utils, \u0027rbd\u0027)"},{"line_number":11807,"context_line":"    @mock.patch.object(rbd_utils, \u0027rados\u0027)"},{"line_number":11808,"context_line":"    def test_cleanup_rbd(self, mock_rados, mock_rbd, mock_destroy_volume):"},{"line_number":11809,"context_line":"        mock_destroy_volume.return_value \u003d None"},{"line_number":11810,"context_line":""},{"line_number":11811,"context_line":"        instance \u003d objects.Instance(**self.test_instance)"},{"line_number":11812,"context_line":"        all_volumes \u003d [uuids.other_instance + \u0027_disk\u0027,"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a89bdaa_09ba4d0a","line":11809,"range":{"start_line":11809,"start_character":8,"end_line":11809,"end_character":47},"updated":"2016-09-07 02:22:19.000000000","message":"You don\u0027t need this","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"94291f5bdac624e238f42b0d40132049dbc31e1c","unresolved":false,"context_lines":[{"line_number":11820,"context_line":"        calls \u003d [mock.call(mock.ANY, instance.uuid + \u0027_disk\u0027),"},{"line_number":11821,"context_line":"                 mock.call(mock.ANY, instance.uuid + \u0027_disk.swap\u0027)]"},{"line_number":11822,"context_line":"        mock_destroy_volume.assert_has_calls(calls)"},{"line_number":11823,"context_line":"        assert mock_destroy_volume.call_count \u003d\u003d 2"},{"line_number":11824,"context_line":""},{"line_number":11825,"context_line":"    @mock.patch.object(objects.Instance, \u0027save\u0027)"},{"line_number":11826,"context_line":"    def test_destroy_undefines_no_undefine_flags(self, mock_save):"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a89bdaa_be88b5c6","line":11823,"range":{"start_line":11823,"start_character":8,"end_line":11823,"end_character":50},"updated":"2016-09-07 02:22:19.000000000","message":"This should be:\n\n self.assertEqual(2, mock_destroy_volume.call_count)","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"94291f5bdac624e238f42b0d40132049dbc31e1c","unresolved":false,"context_lines":[{"line_number":11821,"context_line":"                 mock.call(mock.ANY, instance.uuid + \u0027_disk.swap\u0027)]"},{"line_number":11822,"context_line":"        mock_destroy_volume.assert_has_calls(calls)"},{"line_number":11823,"context_line":"        assert mock_destroy_volume.call_count \u003d\u003d 2"},{"line_number":11824,"context_line":""},{"line_number":11825,"context_line":"    @mock.patch.object(objects.Instance, \u0027save\u0027)"},{"line_number":11826,"context_line":"    def test_destroy_undefines_no_undefine_flags(self, mock_save):"},{"line_number":11827,"context_line":"        mock \u003d self.mox.CreateMock(fakelibvirt.virDomain)"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a89bdaa_648140c3","line":11824,"updated":"2016-09-07 02:22:19.000000000","message":"Add a test that will run the real resize reverting path:\n\n @mock.patch.object(rbd_utils.RBDDriver, \u0027_destroy_volume\u0027)\n @mock.patch.object(rbd_utils, \u0027rbd\u0027)\n @mock.patch.object(rbd_utils, \u0027rados\u0027)\n def test_cleanup_rbd_resize_reverting(self, mock_rados, mock_rbd,\n                                       mock_destroy_volume):\n     instance \u003d objects.Instance(**self.test_instance)\n     instance.task_state \u003d task_states.RESIZE_REVERTING\n     all_volumes \u003d [uuids.other_instance + \u0027_disk\u0027,\n                    uuids.other_instance + \u0027_disk.local\u0027,\n                    instance.uuid + \u0027_disk\u0027,\n                    instance.uuid + \u0027_disk.local\u0027]\n     mock_rbd.RBD.return_value.list.return_value \u003d all_volumes\n     drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True)\n     drvr._cleanup_rbd(instance)\n     mock_destroy_volume.assert_called_once_with(\n         mock.ANY, instance.uuid + \u0027_disk.local\u0027)","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"94291f5bdac624e238f42b0d40132049dbc31e1c","unresolved":false,"context_lines":[{"line_number":16504,"context_line":"    @mock.patch.object(libvirt_utils, \u0027get_instance_path\u0027)"},{"line_number":16505,"context_line":"    @mock.patch.object(libvirt_utils, \u0027load_file\u0027)"},{"line_number":16506,"context_line":"    @mock.patch.object(host.Host, \"get_domain\")"},{"line_number":16507,"context_line":"    def test_unrescue(self, mock_get_domain, mock_load_file,"},{"line_number":16508,"context_line":"                                           mock_get_instance_path):"},{"line_number":16509,"context_line":"        dummyxml \u003d (\"\u003cdomain type\u003d\u0027kvm\u0027\u003e\u003cname\u003einstance-0000000a\u003c/name\u003e\""},{"line_number":16510,"context_line":"                    \"\u003cdevices\u003e\""}],"source_content_type":"text/x-python","patch_set":14,"id":"9a89bdaa_a99881c1","line":16507,"range":{"start_line":16507,"start_character":8,"end_line":16507,"end_character":21},"updated":"2016-09-07 02:22:19.000000000","message":"Make this private i.e. _test_unrescue(self, instance, ...)","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"94291f5bdac624e238f42b0d40132049dbc31e1c","unresolved":false,"context_lines":[{"line_number":16557,"context_line":"            self.assertEqual(1, mock_rmtree.call_count)"},{"line_number":16558,"context_line":"            self.assertEqual(rescue_dir, mock_rmtree.call_args_list[0][0][0])"},{"line_number":16559,"context_line":"            self.assertEqual(rescue_file, mock_del.call_args_list[1][0][0])"},{"line_number":16560,"context_line":"            mock_remove_volumes.assert_called_once_with([\u0027lvm.rescue\u0027])"},{"line_number":16561,"context_line":""},{"line_number":16562,"context_line":"    @mock.patch("},{"line_number":16563,"context_line":"        \u0027nova.virt.libvirt.driver.LibvirtDriver._build_device_metadata\u0027,"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a89bdaa_695599f8","line":16560,"updated":"2016-09-07 02:22:19.000000000","message":"Add two tests:\n\n def test_unrescue(self):\n     instance \u003d objects.Instance(uuid\u003duuids.instance, id\u003d1)\n     self._test_unrescue(instance)\n\n\n @mock.patch.object(rbd_utils.RBDDriver, \u0027_destroy_volume\u0027)\n @mock.patch.object(rbd_utils, \u0027rbd\u0027)\n @mock.patch.object(rbd_utils, \u0027rados\u0027)\n def test_unrescue_rbd(self, mock_rados, mock_rbd, mock_destroy_volume):\n     self.flags(images_type\u003d\u0027rbd\u0027, group\u003d\u0027libvirt\u0027)\n     instance \u003d objects.Instance(uuid\u003duuids.instance, id\u003d1)\n     self._test_unrescue(instance)\n     all_volumes \u003d [uuids.other_instance + \u0027_disk\u0027,\n                    uuids.other_instance + \u0027_disk.rescue\u0027,\n                    instance.uuid + \u0027_disk\u0027,\n                    instance.uuid + \u0027_disk.rescue\u0027]\n     mock_rbd.RBD.return_value.list.return_value \u003d all_volumes\n     self._test_unrescue(instance)\n     mock_destroy_volume.assert_called_once_with(\n         mock.ANY, instance.uuid + \u0027_disk.rescue\u0027)","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"e2be8cf4b56baaafbd5d464358ab6911ed6758b5","unresolved":false,"context_lines":[{"line_number":11619,"context_line":"        mock_destroy_volume.assert_has_calls(calls)"},{"line_number":11620,"context_line":"        self.assertEqual(2, mock_destroy_volume.call_count)"},{"line_number":11621,"context_line":""},{"line_number":11622,"context_line":"    @mock.patch.object(rbd_utils.RBDDriver, \u0027_destroy_volume\u0027)"},{"line_number":11623,"context_line":"    @mock.patch.object(rbd_utils, \u0027rbd\u0027)"},{"line_number":11624,"context_line":"    @mock.patch.object(rbd_utils, \u0027rados\u0027)"},{"line_number":11625,"context_line":"    def test_cleanup_rbd_resize_reverting(self, mock_rados, mock_rbd,"},{"line_number":11626,"context_line":"                                          mock_destroy_volume):"},{"line_number":11627,"context_line":"        instance \u003d objects.Instance(**self.test_instance)"}],"source_content_type":"text/x-python","patch_set":17,"id":"7a8ec9b2_30b5547d","line":11624,"range":{"start_line":11622,"start_character":0,"end_line":11624,"end_character":42},"updated":"2016-09-18 14:36:46.000000000","message":"This doesn\u0027t work, is it clearing the initial _destroy_volume mock?\n\nhttp://logs.openstack.org/28/314928/17/check/gate-nova-python27-db-ubuntu-xenial/8014be8/console.html.gz#_2016-09-15_12_41_52_401189","commit_id":"de8141b2965fdabe2c39b33deee46dfec4657132"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"e2be8cf4b56baaafbd5d464358ab6911ed6758b5","unresolved":false,"context_lines":[{"line_number":16415,"context_line":"    @mock.patch.object(libvirt_utils, \u0027get_instance_path\u0027)"},{"line_number":16416,"context_line":"    @mock.patch.object(libvirt_utils, \u0027load_file\u0027)"},{"line_number":16417,"context_line":"    @mock.patch.object(host.Host, \"get_domain\")"},{"line_number":16418,"context_line":"    def _test_unrescue(self, mock_get_domain, mock_load_file,"},{"line_number":16419,"context_line":"                                           mock_get_instance_path):"},{"line_number":16420,"context_line":"        dummyxml \u003d (\"\u003cdomain type\u003d\u0027kvm\u0027\u003e\u003cname\u003einstance-0000000a\u003c/name\u003e\""},{"line_number":16421,"context_line":"                    \"\u003cdevices\u003e\""},{"line_number":16422,"context_line":"                    \"\u003cdisk type\u003d\u0027block\u0027 device\u003d\u0027disk\u0027\u003e\""}],"source_content_type":"text/x-python","patch_set":17,"id":"7a8ec9b2_d048b0b5","line":16419,"range":{"start_line":16418,"start_character":0,"end_line":16419,"end_character":67},"updated":"2016-09-18 14:36:46.000000000","message":"Missing an instance arg.\n\nhttp://logs.openstack.org/28/314928/17/check/gate-nova-python27-db-ubuntu-xenial/8014be8/console.html.gz#_2016-09-15_12_41_52_400817","commit_id":"de8141b2965fdabe2c39b33deee46dfec4657132"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"e2be8cf4b56baaafbd5d464358ab6911ed6758b5","unresolved":false,"context_lines":[{"line_number":16425,"context_line":"                    \"\u003c/devices\u003e\u003c/domain\u003e\")"},{"line_number":16426,"context_line":""},{"line_number":16427,"context_line":"        mock_get_instance_path.return_value \u003d \u0027/path\u0027"},{"line_number":16428,"context_line":"        instance \u003d objects.Instance(uuid\u003duuids.instance, id\u003d1)"},{"line_number":16429,"context_line":"        fake_dom \u003d FakeVirtDomain(fake_xml\u003ddummyxml)"},{"line_number":16430,"context_line":"        mock_get_domain.return_value \u003d fake_dom"},{"line_number":16431,"context_line":"        mock_load_file.return_value \u003d \"fake_unrescue_xml\""}],"source_content_type":"text/x-python","patch_set":17,"id":"7a8ec9b2_f0c92c27","line":16428,"range":{"start_line":16428,"start_character":0,"end_line":16428,"end_character":62},"updated":"2016-09-18 14:36:46.000000000","message":"Also need to remove this.","commit_id":"de8141b2965fdabe2c39b33deee46dfec4657132"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"b104459e7a9120847ce4a0b83af5b21b1b002d4d","unresolved":false,"context_lines":[{"line_number":2539,"context_line":"        lvm.remove_volumes([lvmdisk for lvmdisk in self._lvm_disks(instance)"},{"line_number":2540,"context_line":"                                if lvmdisk.endswith(\u0027.rescue\u0027)])"},{"line_number":2541,"context_line":""},{"line_number":2542,"context_line":"        if CONF.libvirt.images_type \u003d\u003d \u0027rbd\u0027:"},{"line_number":2543,"context_line":"            rescue_disk \u003d \u0027{0}_disk.rescue\u0027.format(instance[\u0027uuid\u0027])"},{"line_number":2544,"context_line":"            self._get_rbd_driver().destroy_volume(rescue_disk)"},{"line_number":2545,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"bab6814e_c0922a57","line":2542,"updated":"2016-05-18 14:14:46.000000000","message":"I really don\u0027t like this, because this proliferation of backend-specific behaviour in driver.py is very hard to maintain.\n\nThere must be a better solution. Can you explain how this disk is removed for other backends, and why the rbd disk isn\u0027t removed in the same way? My immediate reaction is that we should fix that rather than implement yet another workaround.","commit_id":"cef3df164bdf71419ae76bab3910e86c4385f9d0"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"b151ab3293a2be6ff353ea28d905c8f7004cb2c7","unresolved":false,"context_lines":[{"line_number":2539,"context_line":"        lvm.remove_volumes([lvmdisk for lvmdisk in self._lvm_disks(instance)"},{"line_number":2540,"context_line":"                                if lvmdisk.endswith(\u0027.rescue\u0027)])"},{"line_number":2541,"context_line":"        if CONF.libvirt.images_type \u003d\u003d \u0027rbd\u0027:"},{"line_number":2542,"context_line":"            LibvirtDriver._get_rbd_driver().cleanup_rescue_volumes(instance)"},{"line_number":2543,"context_line":""},{"line_number":2544,"context_line":"    def poll_rebooting_instances(self, timeout, instances):"},{"line_number":2545,"context_line":"        pass"}],"source_content_type":"text/x-python","patch_set":5,"id":"9abb7d3a_a4d3d85e","line":2542,"updated":"2016-06-02 10:01:04.000000000","message":"I still don\u0027t like it, but we\u0027ve explored the alternatives and they\u0027re all worse for now. Lets do it.","commit_id":"23cb5324d9debfa5771bfa21d9f9de5d4ad1eaca"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"b272b4f3e8ad2a3874bf9f126c529d8935fad963","unresolved":false,"context_lines":[{"line_number":992,"context_line":"                rbd_user\u003dCONF.libvirt.rbd_user)"},{"line_number":993,"context_line":""},{"line_number":994,"context_line":"    def _cleanup_rbd(self, instance):"},{"line_number":995,"context_line":"        if instance.task_state \u003d\u003d task_states.RESIZE_REVERTING:"},{"line_number":996,"context_line":"            filter_fn \u003d lambda disk: (disk.startwith(instance.uuid) and"},{"line_number":997,"context_line":"                                      disk.endswith(\u0027disk.local\u0027))"},{"line_number":998,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":6,"id":"7aa08908_e66409ba","line":995,"updated":"2016-06-08 11:09:06.000000000","message":"I was going to say this is badly in need of a comment. There is a comment on the original (in rbd_utils). Could you please move it over, too?","commit_id":"6b39d02dfb15d9e1c55769104f0f8dabf3f6432c"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"b272b4f3e8ad2a3874bf9f126c529d8935fad963","unresolved":false,"context_lines":[{"line_number":2545,"context_line":"                                if lvmdisk.endswith(\u0027.rescue\u0027)])"},{"line_number":2546,"context_line":"        if CONF.libvirt.images_type \u003d\u003d \u0027rbd\u0027:"},{"line_number":2547,"context_line":"            filter_fn \u003d lambda disk: (disk.startswith(instance.uuid) and"},{"line_number":2548,"context_line":"                                      disk.endswith(\u0027rescue\u0027))"},{"line_number":2549,"context_line":"            LibvirtDriver._get_rbd_driver().cleanup_volumes(filter_fn)"},{"line_number":2550,"context_line":""},{"line_number":2551,"context_line":"    def poll_rebooting_instances(self, timeout, instances):"}],"source_content_type":"text/x-python","patch_set":6,"id":"7aa08908_46551df8","line":2548,"updated":"2016-06-08 11:09:06.000000000","message":"nit: .rescue would be more consistent here. I don\u0027t think this could ever cause a problem in practise, though, so only worth fixing if you\u0027re updating anyway.","commit_id":"6b39d02dfb15d9e1c55769104f0f8dabf3f6432c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"07c861ddc96d7f63d6ed34828c65c194a79d083c","unresolved":false,"context_lines":[{"line_number":2550,"context_line":"        if CONF.libvirt.images_type \u003d\u003d \u0027rbd\u0027:"},{"line_number":2551,"context_line":"            filter_fn \u003d lambda disk: (disk.startswith(instance.uuid) and"},{"line_number":2552,"context_line":"                                      disk.endswith(\u0027.rescue\u0027))"},{"line_number":2553,"context_line":"            LibvirtDriver._get_rbd_driver().cleanup_volumes(filter_fn)"},{"line_number":2554,"context_line":""},{"line_number":2555,"context_line":"    def poll_rebooting_instances(self, timeout, instances):"},{"line_number":2556,"context_line":"        pass"}],"source_content_type":"text/x-python","patch_set":7,"id":"7aa08908_cd537e6b","line":2553,"updated":"2016-06-09 22:49:40.000000000","message":"There isn\u0027t any test coverage of this. It looks like you could augment test_unrescue in test_driver.py to set flags to images_type \u0027rbd\u0027 and then assert cleanup_volumes was called with the right filter function","commit_id":"375bb0c1c4f03a57a699ab2cb4938311cdc0e292"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"9629fa4700640f6280b436c293ec7d342122b7ca","unresolved":false,"context_lines":[{"line_number":1079,"context_line":"        # filter out non-ephemerals from the list"},{"line_number":1080,"context_line":"        if instance.task_state \u003d\u003d task_states.RESIZE_REVERTING:"},{"line_number":1081,"context_line":"            filter_fn \u003d lambda disk: (disk.startwith(instance.uuid) and"},{"line_number":1082,"context_line":"                                      disk.endswith(\u0027disk.local\u0027))"},{"line_number":1083,"context_line":"        else:"},{"line_number":1084,"context_line":"            filter_fn \u003d lambda disk: disk.startswith(instance.uuid)"},{"line_number":1085,"context_line":"        LibvirtDriver._get_rbd_driver().cleanup_volumes(filter_fn)"}],"source_content_type":"text/x-python","patch_set":13,"id":"fa7ab95a_5f0af68a","line":1082,"updated":"2016-08-30 13:06:08.000000000","message":"Side-note: This doesn\u0027t filter out ephemerals, because ephemerals are never called \u0027disk.local\u0027 in newly created instances. They\u0027re called \u0027disk.ephX\u0027.\n\nIt\u0027s not an issue in your patch as this is just code motion, but if you\u0027re unfortunate enough to have to respin this patch, could you add a comment to this effect? In either case, don\u0027t try to fix it in this patch.","commit_id":"ea3b4db9c7d0e026529831e03ed10cd1dfc78a9f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"94291f5bdac624e238f42b0d40132049dbc31e1c","unresolved":false,"context_lines":[{"line_number":1078,"context_line":"        # and none of this is needed (and is, in fact, harmful) so"},{"line_number":1079,"context_line":"        # filter out non-ephemerals from the list"},{"line_number":1080,"context_line":"        if instance.task_state \u003d\u003d task_states.RESIZE_REVERTING:"},{"line_number":1081,"context_line":"            filter_fn \u003d lambda disk: (disk.startwith(instance.uuid) and"},{"line_number":1082,"context_line":"                                      disk.endswith(\u0027disk.local\u0027))"},{"line_number":1083,"context_line":"        else:"},{"line_number":1084,"context_line":"            filter_fn \u003d lambda disk: disk.startswith(instance.uuid)"}],"source_content_type":"text/x-python","patch_set":14,"id":"9a89bdaa_78a3a000","line":1081,"range":{"start_line":1081,"start_character":43,"end_line":1081,"end_character":52},"updated":"2016-09-07 02:22:19.000000000","message":"typo, this should be startswith\n\nThis bug was exposed by the test_cleanup_rbd_resize_reverting I asked for in test_driver.py. In test_rbd.py the filter functions are duplicated, so in test_rbd.py you typed \u0027startswith\u0027 correctly so the test passed. But there was a bug in the driver and we didn\u0027t see it exposed by tests. So unfortunately, we have to test each possible filter function scenario in test_driver.py","commit_id":"4ea4e458910023ed42b08c193f0f0f8e135edd5b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ff97d7b0992b615a6b525f3021064bb5bb58644e","unresolved":false,"context_lines":[{"line_number":2554,"context_line":"                libvirt_utils.file_delete(rescue_file)"},{"line_number":2555,"context_line":"        # cleanup rescue volume"},{"line_number":2556,"context_line":"        lvm.remove_volumes([lvmdisk for lvmdisk in self._lvm_disks(instance)"},{"line_number":2557,"context_line":"                           if lvmdisk.endswith(\u0027.rescue\u0027)])"},{"line_number":2558,"context_line":"        if CONF.libvirt.images_type \u003d\u003d \u0027rbd\u0027:"},{"line_number":2559,"context_line":"            filter_fn \u003d lambda disk: (disk.startswith(instance.uuid) and"},{"line_number":2560,"context_line":"                                      disk.endswith(\u0027.rescue\u0027))"}],"source_content_type":"text/x-python","patch_set":20,"id":"fa6399be_17361d52","line":2557,"updated":"2016-10-12 13:56:57.000000000","message":"This change is unrelated to the patch.","commit_id":"8695ed466dd48e3447d03c773387fb5661aef6c7"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ff97d7b0992b615a6b525f3021064bb5bb58644e","unresolved":false,"context_lines":[{"line_number":2555,"context_line":"        # cleanup rescue volume"},{"line_number":2556,"context_line":"        lvm.remove_volumes([lvmdisk for lvmdisk in self._lvm_disks(instance)"},{"line_number":2557,"context_line":"                           if lvmdisk.endswith(\u0027.rescue\u0027)])"},{"line_number":2558,"context_line":"        if CONF.libvirt.images_type \u003d\u003d \u0027rbd\u0027:"},{"line_number":2559,"context_line":"            filter_fn \u003d lambda disk: (disk.startswith(instance.uuid) and"},{"line_number":2560,"context_line":"                                      disk.endswith(\u0027.rescue\u0027))"},{"line_number":2561,"context_line":"            LibvirtDriver._get_rbd_driver().cleanup_volumes(filter_fn)"}],"source_content_type":"text/x-python","patch_set":20,"id":"fa6399be_f715299b","line":2558,"updated":"2016-10-12 13:56:57.000000000","message":"This is really terrible, but I think it\u0027s the way we handle these things in the rest of the code, so .. meh.","commit_id":"8695ed466dd48e3447d03c773387fb5661aef6c7"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"787c9506db56655856e3a28214e9df91fe48246e","unresolved":false,"context_lines":[{"line_number":2555,"context_line":"        # cleanup rescue volume"},{"line_number":2556,"context_line":"        lvm.remove_volumes([lvmdisk for lvmdisk in self._lvm_disks(instance)"},{"line_number":2557,"context_line":"                           if lvmdisk.endswith(\u0027.rescue\u0027)])"},{"line_number":2558,"context_line":"        if CONF.libvirt.images_type \u003d\u003d \u0027rbd\u0027:"},{"line_number":2559,"context_line":"            filter_fn \u003d lambda disk: (disk.startswith(instance.uuid) and"},{"line_number":2560,"context_line":"                                      disk.endswith(\u0027.rescue\u0027))"},{"line_number":2561,"context_line":"            LibvirtDriver._get_rbd_driver().cleanup_volumes(filter_fn)"}],"source_content_type":"text/x-python","patch_set":20,"id":"fa6399be_81002003","line":2558,"in_reply_to":"fa6399be_f715299b","updated":"2016-10-12 16:40:51.000000000","message":"Yup. Not really possible to fix this in a self-contained way right now.","commit_id":"8695ed466dd48e3447d03c773387fb5661aef6c7"}],"nova/virt/libvirt/imagebackend.py":[{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"a86d1a7226d7f2eedf4cc581162246c948a92b87","unresolved":false,"context_lines":[{"line_number":670,"context_line":"        if not self.sparse:"},{"line_number":671,"context_line":"            self.driver_io \u003d \"native\""},{"line_number":672,"context_line":""},{"line_number":673,"context_line":"        self.rescue_disk \u003d \"%s.%s\" % (self.path, \u0027rescue\u0027)"},{"line_number":674,"context_line":""},{"line_number":675,"context_line":"    def _supports_encryption(self):"},{"line_number":676,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":4,"id":"bab6814e_da04a0c7","line":673,"updated":"2016-05-25 09:30:17.000000000","message":"Why in some classes it is \"rescue_disks\" and in other always only one \"rescue_disk\"?","commit_id":"5f0fba20d3261b8df6bd0a1d142a39d09cbdaca9"}],"nova/virt/libvirt/storage/rbd_utils.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"b151ab3293a2be6ff353ea28d905c8f7004cb2c7","unresolved":false,"context_lines":[{"line_number":361,"context_line":"            for volume in filter(belongs_to_instance, volumes):"},{"line_number":362,"context_line":"                self._destroy_volume(client, volume)"},{"line_number":363,"context_line":""},{"line_number":364,"context_line":"    def cleanup_rescue_volumes(self, instance):"},{"line_number":365,"context_line":"        with RADOSClient(self, self.pool) as client:"},{"line_number":366,"context_line":""},{"line_number":367,"context_line":"            def belongs_to_instance(disk):"}],"source_content_type":"text/x-python","patch_set":5,"id":"9abb7d3a_584acc28","line":364,"updated":"2016-06-02 10:01:04.000000000","message":"Can you make 1 change?\n\nIt\u0027s pretty evil that we\u0027re encoding the disk naming scheme in rbd_utils, and it\u0027s also not great that cleanup_volumes and cleanup_rescue_volumes are both the same function with different filters.\n\nCan you please change the signature of cleanup_volumes to take a filter function as an argument. cleanup_volumes then becomes:\n\n  def cleanup_volumes(self, filter_fn):\n    with RADOSClient(self, self.pool) as client:\n      volumes \u003d rbd.RBD().list(client.ioctx)\n      for volume in filter(filter_fn, volumes):\n        self._destroy_volume(client, volume)\n\ndriver._cleanup_rbd becomes:\n\n  if instance.task_state \u003d\u003d task_states.RESIZE_REVERTING:\n    filter_fn \u003d lambda disk: disk.startswith(instance.uuid) and disk.endswith(\u0027disk.local\u0027)\n  else:\n    filter_fn \u003d lambda disk: disk.startswith(instance.uuid)\n\n  LibvirtDriver._get_rbd_driver().cleanup_volumes(filter_fn)\n\nand similarly for cleaning up rescue volumes. It\u0027s less evil for this to be in driver than in rbd_utils.","commit_id":"23cb5324d9debfa5771bfa21d9f9de5d4ad1eaca"}]}