)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"5295dfc25748533a34fb89c0097a8fa7a2e02d1c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"9c50b407_6d49c9fe","updated":"2022-03-01 16:51:06.000000000","message":"LGTM.","commit_id":"e726c07948138f514706cc69440971a2105c2bc0"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"95af3bb7857f6e40bf0df87491aed3f1533550b8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"7374b941_f007ac9c","updated":"2022-03-01 16:58:51.000000000","message":"Thanks for the review","commit_id":"e726c07948138f514706cc69440971a2105c2bc0"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"c629abc6186d83a03b5102edfdfea51826b801b3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"55e943e4_29ca69b6","updated":"2021-11-11 15:53:18.000000000","message":"The code looks good but we only have tests for create from snapshot case and not cloning volume. would be good to have those as well.","commit_id":"e726c07948138f514706cc69440971a2105c2bc0"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"b234e012c2a9ce057c15793fdf5d7dbc3408e900","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"e253c3dc_e2379382","updated":"2022-05-12 16:24:47.000000000","message":"Two +2s and bunch of +1s, merging!","commit_id":"e726c07948138f514706cc69440971a2105c2bc0"}],"cinder/tests/unit/volume/drivers/test_rbd.py":[{"author":{"_account_id":32266,"name":"Venkata krishna Thumu","display_name":"VenkataKrishna","email":"venkata.krishna.reddy@ibm.com","username":"venkatakrishnathumu","status":"Active"},"change_message_id":"ab8edde1fdd2bf8c7476b8b73f4370c0eeea85d7","unresolved":true,"context_lines":[{"line_number":963,"context_line":"        volume_get_by_id.return_value \u003d self.volume_a"},{"line_number":964,"context_line":""},{"line_number":965,"context_line":"        snapshot \u003d mock.Mock(volume_name\u003d\u0027volume-name\u0027,"},{"line_number":966,"context_line":"                             volume_size\u003dself.temp_volume.size)"},{"line_number":967,"context_line":"        self.cfg.rbd_flatten_volume_from_snapshot \u003d True"},{"line_number":968,"context_line":""},{"line_number":969,"context_line":"        self.driver.create_volume_from_snapshot(self.volume_a, snapshot)"}],"source_content_type":"text/x-python","patch_set":11,"id":"93756336_93488add","line":966,"updated":"2021-08-30 13:47:47.000000000","message":"Is it better to take the size of volume_a here ?","commit_id":"c0877ae7f7db7ddb792f0c61f87341277ddc223b"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"703f5fd2d35684d6fa70bde7a3f990d2c02bc123","unresolved":true,"context_lines":[{"line_number":963,"context_line":"        volume_get_by_id.return_value \u003d self.volume_a"},{"line_number":964,"context_line":""},{"line_number":965,"context_line":"        snapshot \u003d mock.Mock(volume_name\u003d\u0027volume-name\u0027,"},{"line_number":966,"context_line":"                             volume_size\u003dself.temp_volume.size)"},{"line_number":967,"context_line":"        self.cfg.rbd_flatten_volume_from_snapshot \u003d True"},{"line_number":968,"context_line":""},{"line_number":969,"context_line":"        self.driver.create_volume_from_snapshot(self.volume_a, snapshot)"}],"source_content_type":"text/x-python","patch_set":11,"id":"ab285cdb_c9aef0dc","line":966,"in_reply_to":"93756336_93488add","updated":"2021-08-30 17:10:30.000000000","message":"Good catch!","commit_id":"c0877ae7f7db7ddb792f0c61f87341277ddc223b"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9bba5fb9787581929d6be95035225dd8e7a79004","unresolved":false,"context_lines":[{"line_number":963,"context_line":"        volume_get_by_id.return_value \u003d self.volume_a"},{"line_number":964,"context_line":""},{"line_number":965,"context_line":"        snapshot \u003d mock.Mock(volume_name\u003d\u0027volume-name\u0027,"},{"line_number":966,"context_line":"                             volume_size\u003dself.temp_volume.size)"},{"line_number":967,"context_line":"        self.cfg.rbd_flatten_volume_from_snapshot \u003d True"},{"line_number":968,"context_line":""},{"line_number":969,"context_line":"        self.driver.create_volume_from_snapshot(self.volume_a, snapshot)"}],"source_content_type":"text/x-python","patch_set":11,"id":"53a570ea_90124080","line":966,"in_reply_to":"ab285cdb_c9aef0dc","updated":"2022-02-11 13:47:01.000000000","message":"Done","commit_id":"c0877ae7f7db7ddb792f0c61f87341277ddc223b"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"c629abc6186d83a03b5102edfdfea51826b801b3","unresolved":true,"context_lines":[{"line_number":936,"context_line":""},{"line_number":937,"context_line":"        self.assertTrue(self.driver._clone_v2_api_checked)"},{"line_number":938,"context_line":""},{"line_number":939,"context_line":"    @common_mocks"},{"line_number":940,"context_line":"    @mock.patch(\u0027cinder.objects.Volume.get_by_id\u0027)"},{"line_number":941,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_get_stripe_unit\u0027,"},{"line_number":942,"context_line":"                       mock.Mock(return_value\u003d4194304))"},{"line_number":943,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_resize\u0027, mock.Mock())"},{"line_number":944,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_flatten\u0027)"},{"line_number":945,"context_line":"    def test_create_temp_vol_from_snap(self, flatten_mock, volume_get_by_id):"},{"line_number":946,"context_line":"        volume_get_by_id.return_value \u003d self.temp_volume"},{"line_number":947,"context_line":""},{"line_number":948,"context_line":"        snapshot \u003d mock.Mock(volume_name\u003d\u0027volume-name\u0027,"},{"line_number":949,"context_line":"                             volume_size\u003dself.temp_volume.size)"},{"line_number":950,"context_line":"        # This is a temp vol so this option will be ignored and won\u0027t flatten"},{"line_number":951,"context_line":"        self.cfg.rbd_flatten_volume_from_snapshot \u003d True"},{"line_number":952,"context_line":""},{"line_number":953,"context_line":"        self.driver.create_volume_from_snapshot(self.temp_volume, snapshot)"},{"line_number":954,"context_line":"        flatten_mock.assert_not_called()"},{"line_number":955,"context_line":""},{"line_number":956,"context_line":"    @common_mocks"},{"line_number":957,"context_line":"    @mock.patch(\u0027cinder.objects.Volume.get_by_id\u0027)"},{"line_number":958,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_get_stripe_unit\u0027,"},{"line_number":959,"context_line":"                       mock.Mock(return_value\u003d4194304))"},{"line_number":960,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_resize\u0027, mock.Mock())"},{"line_number":961,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_flatten\u0027)"},{"line_number":962,"context_line":"    def test_create_vol_from_snap(self, flatten_mock, volume_get_by_id):"},{"line_number":963,"context_line":"        volume_get_by_id.return_value \u003d self.volume_a"},{"line_number":964,"context_line":""},{"line_number":965,"context_line":"        snapshot \u003d mock.Mock(volume_name\u003d\u0027volume-name\u0027,"},{"line_number":966,"context_line":"                             volume_size\u003dself.volume_a.size)"},{"line_number":967,"context_line":"        self.cfg.rbd_flatten_volume_from_snapshot \u003d True"},{"line_number":968,"context_line":""},{"line_number":969,"context_line":"        self.driver.create_volume_from_snapshot(self.volume_a, snapshot)"},{"line_number":970,"context_line":"        flatten_mock.assert_called_once_with(self.cfg.rbd_pool,"},{"line_number":971,"context_line":"                                             self.volume_a.name)"},{"line_number":972,"context_line":""},{"line_number":973,"context_line":"    @common_mocks"},{"line_number":974,"context_line":"    @mock.patch(\u0027cinder.objects.Volume.get_by_id\u0027)"}],"source_content_type":"text/x-python","patch_set":12,"id":"a5eb21cd_adc2c137","line":971,"range":{"start_line":939,"start_character":0,"end_line":971,"end_character":64},"updated":"2021-11-11 15:53:18.000000000","message":"I think we need similar tests for create_cloned_volume as well","commit_id":"e726c07948138f514706cc69440971a2105c2bc0"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"5295dfc25748533a34fb89c0097a8fa7a2e02d1c","unresolved":false,"context_lines":[{"line_number":936,"context_line":""},{"line_number":937,"context_line":"        self.assertTrue(self.driver._clone_v2_api_checked)"},{"line_number":938,"context_line":""},{"line_number":939,"context_line":"    @common_mocks"},{"line_number":940,"context_line":"    @mock.patch(\u0027cinder.objects.Volume.get_by_id\u0027)"},{"line_number":941,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_get_stripe_unit\u0027,"},{"line_number":942,"context_line":"                       mock.Mock(return_value\u003d4194304))"},{"line_number":943,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_resize\u0027, mock.Mock())"},{"line_number":944,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_flatten\u0027)"},{"line_number":945,"context_line":"    def test_create_temp_vol_from_snap(self, flatten_mock, volume_get_by_id):"},{"line_number":946,"context_line":"        volume_get_by_id.return_value \u003d self.temp_volume"},{"line_number":947,"context_line":""},{"line_number":948,"context_line":"        snapshot \u003d mock.Mock(volume_name\u003d\u0027volume-name\u0027,"},{"line_number":949,"context_line":"                             volume_size\u003dself.temp_volume.size)"},{"line_number":950,"context_line":"        # This is a temp vol so this option will be ignored and won\u0027t flatten"},{"line_number":951,"context_line":"        self.cfg.rbd_flatten_volume_from_snapshot \u003d True"},{"line_number":952,"context_line":""},{"line_number":953,"context_line":"        self.driver.create_volume_from_snapshot(self.temp_volume, snapshot)"},{"line_number":954,"context_line":"        flatten_mock.assert_not_called()"},{"line_number":955,"context_line":""},{"line_number":956,"context_line":"    @common_mocks"},{"line_number":957,"context_line":"    @mock.patch(\u0027cinder.objects.Volume.get_by_id\u0027)"},{"line_number":958,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_get_stripe_unit\u0027,"},{"line_number":959,"context_line":"                       mock.Mock(return_value\u003d4194304))"},{"line_number":960,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_resize\u0027, mock.Mock())"},{"line_number":961,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_flatten\u0027)"},{"line_number":962,"context_line":"    def test_create_vol_from_snap(self, flatten_mock, volume_get_by_id):"},{"line_number":963,"context_line":"        volume_get_by_id.return_value \u003d self.volume_a"},{"line_number":964,"context_line":""},{"line_number":965,"context_line":"        snapshot \u003d mock.Mock(volume_name\u003d\u0027volume-name\u0027,"},{"line_number":966,"context_line":"                             volume_size\u003dself.volume_a.size)"},{"line_number":967,"context_line":"        self.cfg.rbd_flatten_volume_from_snapshot \u003d True"},{"line_number":968,"context_line":""},{"line_number":969,"context_line":"        self.driver.create_volume_from_snapshot(self.volume_a, snapshot)"},{"line_number":970,"context_line":"        flatten_mock.assert_called_once_with(self.cfg.rbd_pool,"},{"line_number":971,"context_line":"                                             self.volume_a.name)"},{"line_number":972,"context_line":""},{"line_number":973,"context_line":"    @common_mocks"},{"line_number":974,"context_line":"    @mock.patch(\u0027cinder.objects.Volume.get_by_id\u0027)"}],"source_content_type":"text/x-python","patch_set":12,"id":"7167c016_1d9c1caf","line":971,"range":{"start_line":939,"start_character":0,"end_line":971,"end_character":64},"in_reply_to":"5ff0fd94_24c43986","updated":"2022-03-01 16:51:06.000000000","message":"Somehow i missed those changes before, sorry about that.","commit_id":"e726c07948138f514706cc69440971a2105c2bc0"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9bba5fb9787581929d6be95035225dd8e7a79004","unresolved":false,"context_lines":[{"line_number":936,"context_line":""},{"line_number":937,"context_line":"        self.assertTrue(self.driver._clone_v2_api_checked)"},{"line_number":938,"context_line":""},{"line_number":939,"context_line":"    @common_mocks"},{"line_number":940,"context_line":"    @mock.patch(\u0027cinder.objects.Volume.get_by_id\u0027)"},{"line_number":941,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_get_stripe_unit\u0027,"},{"line_number":942,"context_line":"                       mock.Mock(return_value\u003d4194304))"},{"line_number":943,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_resize\u0027, mock.Mock())"},{"line_number":944,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_flatten\u0027)"},{"line_number":945,"context_line":"    def test_create_temp_vol_from_snap(self, flatten_mock, volume_get_by_id):"},{"line_number":946,"context_line":"        volume_get_by_id.return_value \u003d self.temp_volume"},{"line_number":947,"context_line":""},{"line_number":948,"context_line":"        snapshot \u003d mock.Mock(volume_name\u003d\u0027volume-name\u0027,"},{"line_number":949,"context_line":"                             volume_size\u003dself.temp_volume.size)"},{"line_number":950,"context_line":"        # This is a temp vol so this option will be ignored and won\u0027t flatten"},{"line_number":951,"context_line":"        self.cfg.rbd_flatten_volume_from_snapshot \u003d True"},{"line_number":952,"context_line":""},{"line_number":953,"context_line":"        self.driver.create_volume_from_snapshot(self.temp_volume, snapshot)"},{"line_number":954,"context_line":"        flatten_mock.assert_not_called()"},{"line_number":955,"context_line":""},{"line_number":956,"context_line":"    @common_mocks"},{"line_number":957,"context_line":"    @mock.patch(\u0027cinder.objects.Volume.get_by_id\u0027)"},{"line_number":958,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_get_stripe_unit\u0027,"},{"line_number":959,"context_line":"                       mock.Mock(return_value\u003d4194304))"},{"line_number":960,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_resize\u0027, mock.Mock())"},{"line_number":961,"context_line":"    @mock.patch.object(driver.RBDDriver, \u0027_flatten\u0027)"},{"line_number":962,"context_line":"    def test_create_vol_from_snap(self, flatten_mock, volume_get_by_id):"},{"line_number":963,"context_line":"        volume_get_by_id.return_value \u003d self.volume_a"},{"line_number":964,"context_line":""},{"line_number":965,"context_line":"        snapshot \u003d mock.Mock(volume_name\u003d\u0027volume-name\u0027,"},{"line_number":966,"context_line":"                             volume_size\u003dself.volume_a.size)"},{"line_number":967,"context_line":"        self.cfg.rbd_flatten_volume_from_snapshot \u003d True"},{"line_number":968,"context_line":""},{"line_number":969,"context_line":"        self.driver.create_volume_from_snapshot(self.volume_a, snapshot)"},{"line_number":970,"context_line":"        flatten_mock.assert_called_once_with(self.cfg.rbd_pool,"},{"line_number":971,"context_line":"                                             self.volume_a.name)"},{"line_number":972,"context_line":""},{"line_number":973,"context_line":"    @common_mocks"},{"line_number":974,"context_line":"    @mock.patch(\u0027cinder.objects.Volume.get_by_id\u0027)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5ff0fd94_24c43986","line":971,"range":{"start_line":939,"start_character":0,"end_line":971,"end_character":64},"in_reply_to":"a5eb21cd_adc2c137","updated":"2022-02-11 13:47:01.000000000","message":"Isn\u0027t that what we have in the test_create_cloned_volume_max_depth?\nThere we have 2 cases:\n\n- Cloning temp volume (like we could do for a backup) \u003d\u003e Doesn\u0027t flatten regardless of the config option (like test_create_temp_vol_from_snap). Assert on L1350.\n- Cloning volume with max depth \u003d\u003e Flattens (like test_create_temp_vol_from_snap). Assert on L1342.","commit_id":"e726c07948138f514706cc69440971a2105c2bc0"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"5295dfc25748533a34fb89c0097a8fa7a2e02d1c","unresolved":true,"context_lines":[{"line_number":1339,"context_line":"                    self.mock_proxy.assert_called_once_with("},{"line_number":1340,"context_line":"                        self.driver, dest_vol.name,"},{"line_number":1341,"context_line":"                        client\u003dclient, ioctx\u003dclient.ioctx)"},{"line_number":1342,"context_line":"                    proxy.flatten.assert_called_once_with()"},{"line_number":1343,"context_line":""},{"line_number":1344,"context_line":"                else:"},{"line_number":1345,"context_line":"                    self.mock_rbd.Image.return_value.unprotect_snap.\\"}],"source_content_type":"text/x-python","patch_set":12,"id":"6fb134a7_41d0fc05","line":1342,"range":{"start_line":1342,"start_character":52,"end_line":1342,"end_character":57},"updated":"2022-03-01 16:51:06.000000000","message":"nit: we can remove \"with\" and go with \"assert_called_once()\"","commit_id":"e726c07948138f514706cc69440971a2105c2bc0"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"95af3bb7857f6e40bf0df87491aed3f1533550b8","unresolved":false,"context_lines":[{"line_number":1339,"context_line":"                    self.mock_proxy.assert_called_once_with("},{"line_number":1340,"context_line":"                        self.driver, dest_vol.name,"},{"line_number":1341,"context_line":"                        client\u003dclient, ioctx\u003dclient.ioctx)"},{"line_number":1342,"context_line":"                    proxy.flatten.assert_called_once_with()"},{"line_number":1343,"context_line":""},{"line_number":1344,"context_line":"                else:"},{"line_number":1345,"context_line":"                    self.mock_rbd.Image.return_value.unprotect_snap.\\"}],"source_content_type":"text/x-python","patch_set":12,"id":"a49c55f7_a93d1fa9","line":1342,"range":{"start_line":1342,"start_character":52,"end_line":1342,"end_character":57},"in_reply_to":"6fb134a7_41d0fc05","updated":"2022-03-01 16:58:51.000000000","message":"It\u0027s not the same thing, the assert_called_once method is the same as self.assertEqual(1, proxy.flatten.call_count), so it doesn\u0027t check the arguments.\n\nBy using the assert_called_once_with method I\u0027m ensuring not only that it\u0027s only called once, but also that no arguments were passed on the call.\n\nYou can confirm this in the python interpreter:\n\n   $ python3\n\n   \u003e\u003e\u003e from unittests import mock\n   \u003e\u003e\u003e m \u003d mock.Mock()\n   \u003e\u003e\u003e m.test(1,2)\n\n   \u003e\u003e\u003e m.test.assert_called_once()\n\n   \u003e\u003e\u003e m.test.assert_called_once_with()\n   Traceback (most recent call last):\n     File \"\u003cstdin\u003e\", line 1, in \u003cmodule\u003e\n     File \"/usr/lib64/python3.9/unittest/mock.py\", line 919, in assert_called_once_with\n       return self.assert_called_with(*args, **kwargs)\n     File \"/usr/lib64/python3.9/unittest/mock.py\", line 907, in assert_called_with\n       raise AssertionError(_error_message()) from cause\n   AssertionError: expected call not found.\n   Expected: test()\n   Actual: test(1, 2)","commit_id":"e726c07948138f514706cc69440971a2105c2bc0"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"5e1b4cf7f6dc63694ea633fdc8f101b7a5dd9543","unresolved":false,"context_lines":[{"line_number":1339,"context_line":"                    self.mock_proxy.assert_called_once_with("},{"line_number":1340,"context_line":"                        self.driver, dest_vol.name,"},{"line_number":1341,"context_line":"                        client\u003dclient, ioctx\u003dclient.ioctx)"},{"line_number":1342,"context_line":"                    proxy.flatten.assert_called_once_with()"},{"line_number":1343,"context_line":""},{"line_number":1344,"context_line":"                else:"},{"line_number":1345,"context_line":"                    self.mock_rbd.Image.return_value.unprotect_snap.\\"}],"source_content_type":"text/x-python","patch_set":12,"id":"f7f898cf_486b69ff","line":1342,"range":{"start_line":1342,"start_character":52,"end_line":1342,"end_character":57},"in_reply_to":"a49c55f7_a93d1fa9","updated":"2022-03-01 17:22:42.000000000","message":"That\u0027s true but I\u0027m not sure if flatten method accepts any parameters, from [1] it doesn\u0027t seem like it does (and at least we don\u0027t call it with any parameters from the rbd driver). I understand this adds an additional check that we don\u0027t pass any (wrong) parameters to the call, just unsure about if it\u0027s a useful assertion or not but I don\u0027t have any issue with this additional check.\n\n[1] https://docs.huihoo.com/ceph/v0.80.5/rbd/librbdpy/index.html#rbd.Image.flatten","commit_id":"e726c07948138f514706cc69440971a2105c2bc0"}]}