)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e0e25048a93ea7e64b00a53cb24db582e3562664","unresolved":false,"context_lines":[{"line_number":19,"context_line":"* _create_image_and_backing now explicitly passes is_block_dev to"},{"line_number":20,"context_line":"  _create_ephemeral, it was previously implicit."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"* The unused context argument is no longer passes to _create_ephemeral"},{"line_number":23,"context_line":"  or _create_swap."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"* Arguments exclusively for template functions are no longer passed to"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"7a77a97e_0220999a","line":22,"updated":"2016-11-22 19:30:35.000000000","message":"s/passes/passed/","commit_id":"ba9f68629a6a61c6d881330ad4fa80ee218f456f"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"caf9a8013b8be30db63e65fedfe589f47c4ef1c4","unresolved":false,"context_lines":[{"line_number":34,"context_line":"or conversion. fetch_image will perform some sanity checks and convert"},{"line_number":35,"context_line":"the image format to raw if required. The additional work done by"},{"line_number":36,"context_line":"fetch_image isn\u0027t strictly necessary for kernel and ramdisk images,"},{"line_number":37,"context_line":"but is not expensive and makes the code simpler."},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"The test change to _create_and_inject_local_root is not a change in"},{"line_number":40,"context_line":"behaviour. It reflects that we now pass the root disk object in rather"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":23,"id":"3a71b18c_2e7751d5","line":37,"updated":"2016-12-08 21:14:55.000000000","message":"++","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"def5e31e4ba71f9be757881619723f48a59d687f","unresolved":false,"context_lines":[{"line_number":10492,"context_line":"        for name, disk in six.iteritems(fake_backend.disks):"},{"line_number":10493,"context_line":"            cache \u003d disk.cache"},{"line_number":10494,"context_line":"            if name in (\u0027kernel\u0027, \u0027ramdisk\u0027):"},{"line_number":10495,"context_line":"                cache.assert_called_once_with("},{"line_number":10496,"context_line":"                    context\u003dself.context, filename\u003dmock.ANY, image_id\u003dmock.ANY,"},{"line_number":10497,"context_line":"                    fetch_func\u003dfake_libvirt_utils.fetch_raw_image)"},{"line_number":10498,"context_line":""},{"line_number":10499,"context_line":"        wantFiles \u003d ["},{"line_number":10500,"context_line":"            {\u0027filename\u0027: kernel_fname,"}],"source_content_type":"text/x-python","patch_set":19,"id":"5a74a57a_12fccbb6","side":"PARENT","line":10497,"range":{"start_line":10495,"start_character":0,"end_line":10497,"end_character":66},"updated":"2016-11-29 15:29:41.000000000","message":"Do we have another tests which check that kernel and ramdisk are created as expected?","commit_id":"1fd6f4b963dfd843a8eeb2936594d1f2f383df76"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"4b3865f935e6b8345f5b4cfb2242d9f6481fecf2","unresolved":false,"context_lines":[{"line_number":10492,"context_line":"        for name, disk in six.iteritems(fake_backend.disks):"},{"line_number":10493,"context_line":"            cache \u003d disk.cache"},{"line_number":10494,"context_line":"            if name in (\u0027kernel\u0027, \u0027ramdisk\u0027):"},{"line_number":10495,"context_line":"                cache.assert_called_once_with("},{"line_number":10496,"context_line":"                    context\u003dself.context, filename\u003dmock.ANY, image_id\u003dmock.ANY,"},{"line_number":10497,"context_line":"                    fetch_func\u003dfake_libvirt_utils.fetch_raw_image)"},{"line_number":10498,"context_line":""},{"line_number":10499,"context_line":"        wantFiles \u003d ["},{"line_number":10500,"context_line":"            {\u0027filename\u0027: kernel_fname,"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_09a10846","side":"PARENT","line":10497,"range":{"start_line":10495,"start_character":0,"end_line":10497,"end_character":66},"in_reply_to":"3a71b18c_1e5078d3","updated":"2016-12-07 09:39:58.000000000","message":"hmm... it\u0027s not good that you remove verification of how kernel/ramdisk disks are created instead of fix it. But i just found that there is not enough test coverage even for the root disk. I changed disk_images[\u0027image_id\u0027] argument to None for call of _try_fetch_image_cache in _create_and_inject_local_root method, and no one test of test_driver failed.\n\nSo i\u0027m inclined to agree to remove this check now, assuming that test coverage for all disk creating will be improved later, closer to final stage of the refactoring.","commit_id":"1fd6f4b963dfd843a8eeb2936594d1f2f383df76"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"04e913754527ca9c24efe587f66f3004c71a45e4","unresolved":false,"context_lines":[{"line_number":10492,"context_line":"        for name, disk in six.iteritems(fake_backend.disks):"},{"line_number":10493,"context_line":"            cache \u003d disk.cache"},{"line_number":10494,"context_line":"            if name in (\u0027kernel\u0027, \u0027ramdisk\u0027):"},{"line_number":10495,"context_line":"                cache.assert_called_once_with("},{"line_number":10496,"context_line":"                    context\u003dself.context, filename\u003dmock.ANY, image_id\u003dmock.ANY,"},{"line_number":10497,"context_line":"                    fetch_func\u003dfake_libvirt_utils.fetch_raw_image)"},{"line_number":10498,"context_line":""},{"line_number":10499,"context_line":"        wantFiles \u003d ["},{"line_number":10500,"context_line":"            {\u0027filename\u0027: kernel_fname,"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_1e5078d3","side":"PARENT","line":10497,"range":{"start_line":10495,"start_character":0,"end_line":10497,"end_character":66},"in_reply_to":"5a74a57a_12fccbb6","updated":"2016-12-06 18:03:13.000000000","message":"It\u0027s still tested below via the slightly more obtuse gotFiles interface. All this was doing was asserting that they were fetched with fetch_raw_image, which is no longer true.","commit_id":"1fd6f4b963dfd843a8eeb2936594d1f2f383df76"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"def5e31e4ba71f9be757881619723f48a59d687f","unresolved":false,"context_lines":[{"line_number":10573,"context_line":"        backend.disks[\u0027disk.swap\u0027].cache.assert_called_once_with("},{"line_number":10574,"context_line":"            fetch_func\u003dmock.ANY, filename\u003d\u0027swap_%i\u0027 % expected,"},{"line_number":10575,"context_line":"            size\u003dexpected * units.Mi, context\u003dself.context,"},{"line_number":10576,"context_line":"            ephemeral_size\u003dmock.ANY)"},{"line_number":10577,"context_line":""},{"line_number":10578,"context_line":"    @mock.patch.object(nova.virt.libvirt.imagebackend.Image, \u0027cache\u0027,"},{"line_number":10579,"context_line":"                       side_effect\u003dexception.ImageNotFound(image_id\u003d\u0027fake-id\u0027))"}],"source_content_type":"text/x-python","patch_set":19,"id":"5a74a57a_b23157a4","line":10576,"range":{"start_line":10576,"start_character":27,"end_line":10576,"end_character":35},"updated":"2016-11-29 15:29:41.000000000","message":"it\u0027s weird","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"4b3865f935e6b8345f5b4cfb2242d9f6481fecf2","unresolved":false,"context_lines":[{"line_number":10573,"context_line":"        backend.disks[\u0027disk.swap\u0027].cache.assert_called_once_with("},{"line_number":10574,"context_line":"            fetch_func\u003dmock.ANY, filename\u003d\u0027swap_%i\u0027 % expected,"},{"line_number":10575,"context_line":"            size\u003dexpected * units.Mi, context\u003dself.context,"},{"line_number":10576,"context_line":"            ephemeral_size\u003dmock.ANY)"},{"line_number":10577,"context_line":""},{"line_number":10578,"context_line":"    @mock.patch.object(nova.virt.libvirt.imagebackend.Image, \u0027cache\u0027,"},{"line_number":10579,"context_line":"                       side_effect\u003dexception.ImageNotFound(image_id\u003d\u0027fake-id\u0027))"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_2cfc12b7","line":10576,"range":{"start_line":10576,"start_character":27,"end_line":10576,"end_character":35},"in_reply_to":"3a71b18c_de70c074","updated":"2016-12-07 09:39:58.000000000","message":"ahh, ok. We do not worry about the value because we need to make sure the parameter is passed only.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"04e913754527ca9c24efe587f66f3004c71a45e4","unresolved":false,"context_lines":[{"line_number":10573,"context_line":"        backend.disks[\u0027disk.swap\u0027].cache.assert_called_once_with("},{"line_number":10574,"context_line":"            fetch_func\u003dmock.ANY, filename\u003d\u0027swap_%i\u0027 % expected,"},{"line_number":10575,"context_line":"            size\u003dexpected * units.Mi, context\u003dself.context,"},{"line_number":10576,"context_line":"            ephemeral_size\u003dmock.ANY)"},{"line_number":10577,"context_line":""},{"line_number":10578,"context_line":"    @mock.patch.object(nova.virt.libvirt.imagebackend.Image, \u0027cache\u0027,"},{"line_number":10579,"context_line":"                       side_effect\u003dexception.ImageNotFound(image_id\u003d\u0027fake-id\u0027))"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_de70c074","line":10576,"range":{"start_line":10576,"start_character":27,"end_line":10576,"end_character":35},"in_reply_to":"5a74a57a_b23157a4","updated":"2016-12-06 18:03:13.000000000","message":"Yup. It\u0027s a disgusting hack to avoid breaking existing code until we replace it.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"760e00276e3ef26ecf92b1c64bf567974c2f8ca9","unresolved":false,"context_lines":[{"line_number":10584,"context_line":"                           block_device_info\u003dbdi, **create_image_kwargs)"},{"line_number":10585,"context_line":""},{"line_number":10586,"context_line":"        backend.mock_create_swap.assert_called_once_with("},{"line_number":10587,"context_line":"            target\u003d\u0027swap_%i\u0027 % expected, swap_mb\u003dexpected)"},{"line_number":10588,"context_line":"        backend.disks[\u0027disk.swap\u0027].cache.assert_called_once_with("},{"line_number":10589,"context_line":"            fetch_func\u003dmock.ANY, filename\u003d\u0027swap_%i\u0027 % expected,"},{"line_number":10590,"context_line":"            size\u003dexpected * units.Mi, context\u003dself.context,"}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_8414cf44","line":10587,"updated":"2016-12-13 20:26:33.000000000","message":"I wondered about not passing context to _create_swap and noticed it takes context\u003dNone and does nothing with it -- we should remove that at some point.","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"33699766851dc30a445e89d0959ad54ca5266db5","unresolved":false,"context_lines":[{"line_number":10584,"context_line":"                           block_device_info\u003dbdi, **create_image_kwargs)"},{"line_number":10585,"context_line":""},{"line_number":10586,"context_line":"        backend.mock_create_swap.assert_called_once_with("},{"line_number":10587,"context_line":"            target\u003d\u0027swap_%i\u0027 % expected, swap_mb\u003dexpected)"},{"line_number":10588,"context_line":"        backend.disks[\u0027disk.swap\u0027].cache.assert_called_once_with("},{"line_number":10589,"context_line":"            fetch_func\u003dmock.ANY, filename\u003d\u0027swap_%i\u0027 % expected,"},{"line_number":10590,"context_line":"            size\u003dexpected * units.Mi, context\u003dself.context,"}],"source_content_type":"text/x-python","patch_set":23,"id":"ba5201f7_7af2e270","line":10587,"in_reply_to":"1a6eadb0_8414cf44","updated":"2017-01-05 16:09:17.000000000","message":"Yeah, this gets dropped in one of my later patches. I forget which one.","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"760e00276e3ef26ecf92b1c64bf567974c2f8ca9","unresolved":false,"context_lines":[{"line_number":10655,"context_line":"        backend.mock_create_ephemeral.assert_called_once_with("},{"line_number":10656,"context_line":"            target\u003dfilename, ephemeral_size\u003d100, fs_label\u003d\u0027ephemeral0\u0027,"},{"line_number":10657,"context_line":"            is_block_dev\u003dmock.sentinel.is_block_dev, os_type\u003d\u0027linux\u0027,"},{"line_number":10658,"context_line":"            specified_fs\u003dNone)"},{"line_number":10659,"context_line":"        backend.disks[\u0027disk.eph0\u0027].cache.assert_called_once_with("},{"line_number":10660,"context_line":"            fetch_func\u003dmock.ANY, context\u003dself.context,"},{"line_number":10661,"context_line":"            filename\u003dfilename, size\u003d100 * units.Gi, ephemeral_size\u003dmock.ANY)"}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_843befd0","line":10658,"updated":"2016-12-13 20:26:33.000000000","message":"Same with _create_ephemeral.","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"caf9a8013b8be30db63e65fedfe589f47c4ef1c4","unresolved":false,"context_lines":[{"line_number":16236,"context_line":"        instance_ref \u003d self._create_instance()"},{"line_number":16237,"context_line":"        disk_images \u003d {\u0027image_id\u0027: None}"},{"line_number":16238,"context_line":""},{"line_number":16239,"context_line":"        root_disk \u003d mock.MagicMock()"},{"line_number":16240,"context_line":"        drvr._create_and_inject_local_root(self.context, instance_ref,"},{"line_number":16241,"context_line":"                                           root_disk, False, \u0027\u0027, disk_images,"},{"line_number":16242,"context_line":"                                           get_injection_info(), None)"}],"source_content_type":"text/x-python","patch_set":23,"id":"3a71b18c_d1283c88","line":16239,"updated":"2016-12-08 21:14:55.000000000","message":"not sure if this needs to be a mock.MagicMock() or whether you can just pass mock.sentinel.root_disk below?","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"53fe8660aac86b6980f91315c5f73ea8b71fc0e0","unresolved":false,"context_lines":[{"line_number":16236,"context_line":"        instance_ref \u003d self._create_instance()"},{"line_number":16237,"context_line":"        disk_images \u003d {\u0027image_id\u0027: None}"},{"line_number":16238,"context_line":""},{"line_number":16239,"context_line":"        root_disk \u003d mock.MagicMock()"},{"line_number":16240,"context_line":"        drvr._create_and_inject_local_root(self.context, instance_ref,"},{"line_number":16241,"context_line":"                                           root_disk, False, \u0027\u0027, disk_images,"},{"line_number":16242,"context_line":"                                           get_injection_info(), None)"}],"source_content_type":"text/x-python","patch_set":23,"id":"3a71b18c_ba7c65ec","line":16239,"in_reply_to":"3a71b18c_d1283c88","updated":"2016-12-09 10:06:26.000000000","message":"IIRC we call methods on it, and sentinels get grumpy if you do that. For the purposes of this test, we don\u0027t actually care about any of those calls.","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"}],"nova/tests/unit/virt/libvirt/test_imagebackend.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e0e25048a93ea7e64b00a53cb24db582e3562664","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                                    mock.sentinel.size,"},{"line_number":118,"context_line":"                                    fallback\u003dmock.sentinel.fallback)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"        cache_name \u003d imagecache.get_cache_fname(self.instance.image_ref)"},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"        self.mock_cache.assert_called_once_with("},{"line_number":123,"context_line":"            mock.ANY, cache_name, context\u003dself.context,"}],"source_content_type":"text/x-python","patch_set":18,"id":"7a77a97e_c2d22134","line":120,"updated":"2016-11-22 19:30:35.000000000","message":"femto-nit: you might consider placing this in the setUp() method and just referencing self.cache_name since you call this in the three test methods above.","commit_id":"ba9f68629a6a61c6d881330ad4fa80ee218f456f"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"760e00276e3ef26ecf92b1c64bf567974c2f8ca9","unresolved":false,"context_lines":[{"line_number":71,"context_line":""},{"line_number":72,"context_line":"    def setUp(self):"},{"line_number":73,"context_line":"        super(RefactorShimTestCase, self).setUp()"},{"line_number":74,"context_line":"        self.context \u003d context.get_admin_context()"},{"line_number":75,"context_line":"        self.instance \u003d objects.Instance(id\u003d1, uuid\u003duuids.instance_id,"},{"line_number":76,"context_line":"                                         image_ref\u003duuids.image_id)"},{"line_number":77,"context_line":""}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_7555a04e","line":74,"range":{"start_line":74,"start_character":31,"end_line":74,"end_character":50},"updated":"2016-12-13 20:26:33.000000000","message":"We have context.get_context() now for avoiding defaulting to admin context everywhere.","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"33699766851dc30a445e89d0959ad54ca5266db5","unresolved":false,"context_lines":[{"line_number":71,"context_line":""},{"line_number":72,"context_line":"    def setUp(self):"},{"line_number":73,"context_line":"        super(RefactorShimTestCase, self).setUp()"},{"line_number":74,"context_line":"        self.context \u003d context.get_admin_context()"},{"line_number":75,"context_line":"        self.instance \u003d objects.Instance(id\u003d1, uuid\u003duuids.instance_id,"},{"line_number":76,"context_line":"                                         image_ref\u003duuids.image_id)"},{"line_number":77,"context_line":""}],"source_content_type":"text/x-python","patch_set":23,"id":"ba5201f7_852d34aa","line":74,"range":{"start_line":74,"start_character":31,"end_line":74,"end_character":50},"in_reply_to":"1a6eadb0_7555a04e","updated":"2017-01-05 16:09:17.000000000","message":"Done","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"760e00276e3ef26ecf92b1c64bf567974c2f8ca9","unresolved":false,"context_lines":[{"line_number":72,"context_line":"    def setUp(self):"},{"line_number":73,"context_line":"        super(RefactorShimTestCase, self).setUp()"},{"line_number":74,"context_line":"        self.context \u003d context.get_admin_context()"},{"line_number":75,"context_line":"        self.instance \u003d objects.Instance(id\u003d1, uuid\u003duuids.instance_id,"},{"line_number":76,"context_line":"                                         image_ref\u003duuids.image_id)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"        self.disk \u003d self.FakeImage(self.instance, \u0027disk\u0027)"}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_b57818f4","line":75,"range":{"start_line":75,"start_character":41,"end_line":75,"end_character":47},"updated":"2016-12-13 20:26:33.000000000","message":"You don\u0027t need to pass id when creating an object. It\u0027s created automatically by instance.create() but not needed for this type of testing.","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"33699766851dc30a445e89d0959ad54ca5266db5","unresolved":false,"context_lines":[{"line_number":72,"context_line":"    def setUp(self):"},{"line_number":73,"context_line":"        super(RefactorShimTestCase, self).setUp()"},{"line_number":74,"context_line":"        self.context \u003d context.get_admin_context()"},{"line_number":75,"context_line":"        self.instance \u003d objects.Instance(id\u003d1, uuid\u003duuids.instance_id,"},{"line_number":76,"context_line":"                                         image_ref\u003duuids.image_id)"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"        self.disk \u003d self.FakeImage(self.instance, \u0027disk\u0027)"}],"source_content_type":"text/x-python","patch_set":23,"id":"ba5201f7_4527ccc9","line":75,"range":{"start_line":75,"start_character":41,"end_line":75,"end_character":47},"in_reply_to":"1a6eadb0_b57818f4","updated":"2017-01-05 16:09:17.000000000","message":"As I\u0027m updating anyway...","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"760e00276e3ef26ecf92b1c64bf567974c2f8ca9","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                                    fallback\u003dmock.sentinel.fallback)"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"        image_hash \u003d imagecache.get_cache_fname(self.instance.image_ref)"},{"line_number":136,"context_line":"        self.mock_cache.assert_has_calls(["},{"line_number":137,"context_line":"            mock.call(mock.ANY, image_hash, context\u003dself.context,"},{"line_number":138,"context_line":"                      image_id\u003dself.instance.image_ref,"},{"line_number":139,"context_line":"                      size\u003dmock.sentinel.size),"}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_30425611","line":136,"range":{"start_line":136,"start_character":24,"end_line":136,"end_character":40},"updated":"2016-12-13 20:26:33.000000000","message":"FYI this isn\u0027t bounded, so self.assertEqual(2, self.mock_cache.call_count) would be needed in addition, to mean you expect only these two calls. Occasionally, I have seen tests pick up on increases in call counts.","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"33699766851dc30a445e89d0959ad54ca5266db5","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                                    fallback\u003dmock.sentinel.fallback)"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"        image_hash \u003d imagecache.get_cache_fname(self.instance.image_ref)"},{"line_number":136,"context_line":"        self.mock_cache.assert_has_calls(["},{"line_number":137,"context_line":"            mock.call(mock.ANY, image_hash, context\u003dself.context,"},{"line_number":138,"context_line":"                      image_id\u003dself.instance.image_ref,"},{"line_number":139,"context_line":"                      size\u003dmock.sentinel.size),"}],"source_content_type":"text/x-python","patch_set":23,"id":"ba5201f7_f02cfc9a","line":136,"range":{"start_line":136,"start_character":24,"end_line":136,"end_character":40},"in_reply_to":"1a6eadb0_30425611","updated":"2017-01-05 16:09:17.000000000","message":"Done","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"caf9a8013b8be30db63e65fedfe589f47c4ef1c4","unresolved":false,"context_lines":[{"line_number":166,"context_line":"            fetch_func\u003dmock.ANY, filename\u003d\u0027fake-cache_name\u0027,"},{"line_number":167,"context_line":"            size\u003dmock.sentinel.size, context\u003dself.context,"},{"line_number":168,"context_line":"            ephemeral_size\u003dmock.ANY)"},{"line_number":169,"context_line":"        func.assert_called_once_with(target\u003dmock.sentinel.target)"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"class _ImageTestCase(object):"}],"source_content_type":"text/x-python","patch_set":23,"id":"3a71b18c_511b4c1c","line":169,"updated":"2016-12-08 21:14:55.000000000","message":"++ good tests.","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e0e25048a93ea7e64b00a53cb24db582e3562664","unresolved":false,"context_lines":[{"line_number":2981,"context_line":"                      disk_images\u003dNone, block_device_info\u003dNone,"},{"line_number":2982,"context_line":"                      fallback_from_host\u003dNone,"},{"line_number":2983,"context_line":"                      ignore_bdi_for_swap\u003dFalse):"},{"line_number":2984,"context_line":"        rescue \u003d suffix \u003d\u003d \u0027.rescue\u0027"},{"line_number":2985,"context_line":"        booted_from_volume \u003d self._is_booted_from_volume("},{"line_number":2986,"context_line":"            instance, disk_mapping)"},{"line_number":2987,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"7a77a97e_427c5120","line":2984,"updated":"2016-11-22 19:30:35.000000000","message":"nit: might want to name this variable \"is_rescue\". Actually didn\u0027t realize this was a boolean until I looked closely at the changed params to _create_and_inject_local_root()","commit_id":"ba9f68629a6a61c6d881330ad4fa80ee218f456f"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"509518d8369fae2fcda42575c8fdd2011bfa7ba0","unresolved":false,"context_lines":[{"line_number":6660,"context_line":"                                           info[\u0027virt_disk_size\u0027])"},{"line_number":6661,"context_line":"            elif info[\u0027backing_file\u0027]:"},{"line_number":6662,"context_line":"                # Creating backing file follows same way as spawning instances."},{"line_number":6663,"context_line":"                cache_name \u003d os.path.basename(info[\u0027backing_file\u0027])"},{"line_number":6664,"context_line":""},{"line_number":6665,"context_line":"                disk \u003d self.image_backend.by_name(instance, instance_disk,"},{"line_number":6666,"context_line":"                                                  CONF.libvirt.images_type)"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_fbec712b","line":6663,"updated":"2016-12-07 21:12:09.000000000","message":"i was confused by this line, because that i thought that backing_file contains full file name","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"}],"nova/virt/libvirt/imagebackend.py":[{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"def5e31e4ba71f9be757881619723f48a59d687f","unresolved":false,"context_lines":[{"line_number":123,"context_line":"        :param source_disk_info: instance_disk_info from a remote host. We"},{"line_number":124,"context_line":"                                 will copy parameters from this data when"},{"line_number":125,"context_line":"                                 creating the disk locally."},{"line_number":126,"context_line":"        :param fallback: A compute host to side-load the output from if it"},{"line_number":127,"context_line":"                         is not already in the image cache."},{"line_number":128,"context_line":"        \"\"\""},{"line_number":129,"context_line":"        # Define a compatibility function which will ignore any arguments"},{"line_number":130,"context_line":"        # passed to it other than target"}],"source_content_type":"text/x-python","patch_set":19,"id":"5a74a57a_1cea22db","line":127,"range":{"start_line":126,"start_character":0,"end_line":127,"end_character":59},"updated":"2016-11-29 15:29:41.000000000","message":"Why do we need this if it is not used?","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"0a669c0fa4f8edf36f9d3f7ec3a603975d8592cb","unresolved":false,"context_lines":[{"line_number":123,"context_line":"        :param source_disk_info: instance_disk_info from a remote host. We"},{"line_number":124,"context_line":"                                 will copy parameters from this data when"},{"line_number":125,"context_line":"                                 creating the disk locally."},{"line_number":126,"context_line":"        :param fallback: A compute host to side-load the output from if it"},{"line_number":127,"context_line":"                         is not already in the image cache."},{"line_number":128,"context_line":"        \"\"\""},{"line_number":129,"context_line":"        # Define a compatibility function which will ignore any arguments"},{"line_number":130,"context_line":"        # passed to it other than target"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_ff8fecc9","line":127,"range":{"start_line":126,"start_character":0,"end_line":127,"end_character":59},"in_reply_to":"3a71b18c_fec73c61","updated":"2016-12-08 06:12:36.000000000","message":"Might we have it as a parameter of func function? Or it is a deal for further refactoring?","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"04e913754527ca9c24efe587f66f3004c71a45e4","unresolved":false,"context_lines":[{"line_number":123,"context_line":"        :param source_disk_info: instance_disk_info from a remote host. We"},{"line_number":124,"context_line":"                                 will copy parameters from this data when"},{"line_number":125,"context_line":"                                 creating the disk locally."},{"line_number":126,"context_line":"        :param fallback: A compute host to side-load the output from if it"},{"line_number":127,"context_line":"                         is not already in the image cache."},{"line_number":128,"context_line":"        \"\"\""},{"line_number":129,"context_line":"        # Define a compatibility function which will ignore any arguments"},{"line_number":130,"context_line":"        # passed to it other than target"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_fec73c61","line":127,"range":{"start_line":126,"start_character":0,"end_line":127,"end_character":59},"in_reply_to":"5a74a57a_1cea22db","updated":"2016-12-06 18:03:13.000000000","message":"The new backend code uses fallback to copy the backing files of ephemeral/swap disks from the source if they\u0027re not present locally. This shim implementation doesn\u0027t use it, but we still need it.\n\nI should add a comment for this, because it\u0027s not obvious.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"def5e31e4ba71f9be757881619723f48a59d687f","unresolved":false,"context_lines":[{"line_number":131,"context_line":"        compat \u003d lambda target, **kwargs: func(target\u003dtarget)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"        if source_disk_info is not None:"},{"line_number":134,"context_line":"            backing_file \u003d source_disk_info.get(\u0027backing_file\u0027)"},{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"}],"source_content_type":"text/x-python","patch_set":19,"id":"5a74a57a_f7f4a1f1","line":135,"range":{"start_line":134,"start_character":0,"end_line":135,"end_character":71},"updated":"2016-11-29 15:29:41.000000000","message":"The only caller passes source_disk_info is _create_images_and_backing. And it passes cache_name \u003d os.path.basename(info[\u0027backing_file\u0027]). So i guess in this case backing_file !\u003d cache_name always.\n\n1 Why do you need to pass full and base file names separately and use this complex logic to get the full name to use it further?\n\n2 Why do you need to use the full name instead of the base name? Do you expect that the file path differs from local cach path, and cache method will use that path?","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"0a669c0fa4f8edf36f9d3f7ec3a603975d8592cb","unresolved":false,"context_lines":[{"line_number":131,"context_line":"        compat \u003d lambda target, **kwargs: func(target\u003dtarget)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"        if source_disk_info is not None:"},{"line_number":134,"context_line":"            backing_file \u003d source_disk_info.get(\u0027backing_file\u0027)"},{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_7f5cbced","line":135,"range":{"start_line":134,"start_character":0,"end_line":135,"end_character":71},"in_reply_to":"3a71b18c_02b8fd73","updated":"2016-12-08 06:12:36.000000000","message":"As i understand there are two cases: (block) live migration, when nothing exists on the target node, and cold migration, when work file (not backing) is already copied onto the target node. In the second cases we could get backing file name from the disk itself. But in the first case we must pass it from outside, you\u0027re right.\n\nHowever, what is the reason to pass both standard cache name and real cache names? We might select the only name outside. Moreover, what is the reason to pass the real cache name as a part of some structure, which is not directly related to imagebackend? This increases internal dependency of code.\n\nProbably it is a deal for your storagepolicy object (or even some strategy classes, we may have a set of hierarchy of such classes, particularly to move into them file coping from driver.py, one hierarchy for different kind of operation: create, cold migration, live migration, etc), which might parse the structure, choose proper name, and pass it here. My concern is that the interface of this new method is too complex. I\u0027d try to simplify it.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"4b3865f935e6b8345f5b4cfb2242d9f6481fecf2","unresolved":false,"context_lines":[{"line_number":131,"context_line":"        compat \u003d lambda target, **kwargs: func(target\u003dtarget)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"        if source_disk_info is not None:"},{"line_number":134,"context_line":"            backing_file \u003d source_disk_info.get(\u0027backing_file\u0027)"},{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_ec813ac4","line":135,"range":{"start_line":134,"start_character":0,"end_line":135,"end_character":71},"in_reply_to":"3a71b18c_5e9bd017","updated":"2016-12-07 09:39:58.000000000","message":"Since backing file name is stored in qcow2 disk, would it be better to get the name from the disk directly? It could be done with a virtual method (we talked about something similar somewhere in another review).\n\nAnd anyway, backing_file here is a full file name with path. Are you sure that cache method handles such value as filename argument correctly?","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"3eff42135c08b24eb2645bc4146c55de4cc80337","unresolved":false,"context_lines":[{"line_number":131,"context_line":"        compat \u003d lambda target, **kwargs: func(target\u003dtarget)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"        if source_disk_info is not None:"},{"line_number":134,"context_line":"            backing_file \u003d source_disk_info.get(\u0027backing_file\u0027)"},{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_ea164554","line":135,"range":{"start_line":134,"start_character":0,"end_line":135,"end_character":71},"in_reply_to":"3a71b18c_6a73154e","updated":"2016-12-08 09:40:12.000000000","message":"Right. But as i\u0027ve mentioned before, in this case we lose them not immediately, because we\u0027ll have to support old instances, started with backing files.\n\nAs for the simplification - i\u0027ll think about it more.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"468879a94c54e02ed90b75ae88a58fac84341562","unresolved":false,"context_lines":[{"line_number":131,"context_line":"        compat \u003d lambda target, **kwargs: func(target\u003dtarget)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"        if source_disk_info is not None:"},{"line_number":134,"context_line":"            backing_file \u003d source_disk_info.get(\u0027backing_file\u0027)"},{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_6a73154e","line":135,"range":{"start_line":134,"start_character":0,"end_line":135,"end_character":71},"in_reply_to":"3a71b18c_7f5cbced","updated":"2016-12-08 09:31:25.000000000","message":"It can\u0027t be simplified without removing functionality, because everything here is used.\n\nIncidentally, as I\u0027ve mentioned before I\u0027d really like to remove backing files for ephemeral and swap disks. If we did that we could lose cache_name, source_disk_info, and fallback.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"df99c542d5b805abca037551538a7a971fba1f6f","unresolved":false,"context_lines":[{"line_number":131,"context_line":"        compat \u003d lambda target, **kwargs: func(target\u003dtarget)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"        if source_disk_info is not None:"},{"line_number":134,"context_line":"            backing_file \u003d source_disk_info.get(\u0027backing_file\u0027)"},{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_cd6d23ce","line":135,"range":{"start_line":134,"start_character":0,"end_line":135,"end_character":71},"in_reply_to":"3a71b18c_ea164554","updated":"2016-12-08 09:43:30.000000000","message":"Please don\u0027t think about it for long, because I want to get on with this. Remember that this is *vastly* simpler than the collection of callbacks which are there now. Even if you have a better idea later, it will be simpler to modify this code than the existing code. Lets not get hung up on this.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"0b6f97c0529d5a42e0bb359f919f7bfc2393b53e","unresolved":false,"context_lines":[{"line_number":131,"context_line":"        compat \u003d lambda target, **kwargs: func(target\u003dtarget)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"        if source_disk_info is not None:"},{"line_number":134,"context_line":"            backing_file \u003d source_disk_info.get(\u0027backing_file\u0027)"},{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_02b8fd73","line":135,"range":{"start_line":134,"start_character":0,"end_line":135,"end_character":71},"in_reply_to":"3a71b18c_ec813ac4","updated":"2016-12-07 10:35:53.000000000","message":"It *can\u0027t* be done here, because we don\u0027t have the qcow2, or its backing file. It\u0027s on the source. Also, it doesn\u0027t exist yet, or we wouldn\u0027t be calling create (mostly true at this point, completely true within a couple of patches).\n\nbacking_file is a basename, not a full path. It comes from get_instance_disk_info on the source host, which gets it from libvirt.utils.get_disk_backing_file, which calls basename on it before returning it.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"04e913754527ca9c24efe587f66f3004c71a45e4","unresolved":false,"context_lines":[{"line_number":131,"context_line":"        compat \u003d lambda target, **kwargs: func(target\u003dtarget)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"        if source_disk_info is not None:"},{"line_number":134,"context_line":"            backing_file \u003d source_disk_info.get(\u0027backing_file\u0027)"},{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_5e9bd017","line":135,"range":{"start_line":134,"start_character":0,"end_line":135,"end_character":71},"in_reply_to":"5a74a57a_f7f4a1f1","updated":"2016-12-06 18:03:13.000000000","message":"Again, nothing makes use of this now, but this is used later:\n\nhttps://review.openstack.org/#/c/337160/14/nova/virt/libvirt/driver.py\n\nThis is for compatibility with very old instances which used a different naming scheme for backing files. This handles the case when the source tells us that the backing file isn\u0027t what we expect.\n\nLater still this logic moves into Qcow2, and everything else can ignore it.\n\nAgain, I\u0027ll add a comment about this because it\u0027s not obvious.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"52b6c91fa7c4411838cd127b9679c1bc408a65d2","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        # backend. It is not used, as it is already encapsulated in the"},{"line_number":143,"context_line":"        # callback function we were passed."},{"line_number":144,"context_line":"        self.cache(fetch_func\u003dcompat, filename\u003dcache_name, size\u003dsize,"},{"line_number":145,"context_line":"                   context\u003dcontext, ephemeral_size\u003d\u0027dummy\u0027)"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    def create_from_image(self, context, image_id, size,"},{"line_number":148,"context_line":"                          source_disk_info\u003dNone, fallback\u003dNone):"}],"source_content_type":"text/x-python","patch_set":19,"id":"5a74a57a_feb55eec","line":145,"range":{"start_line":145,"start_character":36,"end_line":145,"end_character":58},"updated":"2016-11-29 14:22:14.000000000","message":"This brings the compatibility in the case of ephemeral, but breaks the case of swap. Currently Lvm backend processes swap in a branch together with root disk (see L799-810), using standard cache. I.e. _create_swap_ prepares a formatted image, which is then being copied into lvm volume. However with this change results of _create_swap is nowhere used.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"4b3865f935e6b8345f5b4cfb2242d9f6481fecf2","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        # backend. It is not used, as it is already encapsulated in the"},{"line_number":143,"context_line":"        # callback function we were passed."},{"line_number":144,"context_line":"        self.cache(fetch_func\u003dcompat, filename\u003dcache_name, size\u003dsize,"},{"line_number":145,"context_line":"                   context\u003dcontext, ephemeral_size\u003d\u0027dummy\u0027)"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    def create_from_image(self, context, image_id, size,"},{"line_number":148,"context_line":"                          source_disk_info\u003dNone, fallback\u003dNone):"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_56cc90ba","line":145,"range":{"start_line":145,"start_character":36,"end_line":145,"end_character":58},"in_reply_to":"3a71b18c_1915f2e3","updated":"2016-12-07 09:39:58.000000000","message":"hmm... looks like that we both (at least i) are wrong\n\n_create_swap cannot write directly to a disk, because it undoubtedly creates a new raw disk on a passed path.\n\n    def _create_swap(target, swap_mb, context\u003dNone):\n        \"\"\"Create a swap file of specified size.\"\"\"\n        libvirt_utils.create_image(\u0027raw\u0027, target, \u0027%dM\u0027 % swap_mb)\n        utils.mkfs(\u0027swap\u0027, target)\n\nSince self.path is passed on L805, it would fail, but fetch_func_sync saves. Because it does not call _create_swap if the passed path exists. And the path exists - it is just created with lvm.create_volume.\n\nSo that the broken compatibility is swap format, which is now absent. However i do not think that such level of incompatibility is high enough to block the patch. So i\u0027ll remove my -1.\n\nBut i still think that it would be better to place the next patch (https://review.openstack.org/#/c/333263/) before this in the series.\n\nAs for you note, i agree with that. However it does not bring simplification of the code immediately, because we have to support old instances.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"0b6f97c0529d5a42e0bb359f919f7bfc2393b53e","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        # backend. It is not used, as it is already encapsulated in the"},{"line_number":143,"context_line":"        # callback function we were passed."},{"line_number":144,"context_line":"        self.cache(fetch_func\u003dcompat, filename\u003dcache_name, size\u003dsize,"},{"line_number":145,"context_line":"                   context\u003dcontext, ephemeral_size\u003d\u0027dummy\u0027)"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    def create_from_image(self, context, image_id, size,"},{"line_number":148,"context_line":"                          source_disk_info\u003dNone, fallback\u003dNone):"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_9dd54e1d","line":145,"range":{"start_line":145,"start_character":36,"end_line":145,"end_character":58},"in_reply_to":"3a71b18c_56cc90ba","updated":"2016-12-07 10:35:53.000000000","message":"I can change the order, but they\u0027ll still be broken as noted in the next patch. I don\u0027t want to fix this here, because it just creates busy work. Lets just concede that Lvm was already broken and it\u0027s now broken differently, but by rationalising we can fix it soon :)","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"04e913754527ca9c24efe587f66f3004c71a45e4","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        # backend. It is not used, as it is already encapsulated in the"},{"line_number":143,"context_line":"        # callback function we were passed."},{"line_number":144,"context_line":"        self.cache(fetch_func\u003dcompat, filename\u003dcache_name, size\u003dsize,"},{"line_number":145,"context_line":"                   context\u003dcontext, ephemeral_size\u003d\u0027dummy\u0027)"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    def create_from_image(self, context, image_id, size,"},{"line_number":148,"context_line":"                          source_disk_info\u003dNone, fallback\u003dNone):"}],"source_content_type":"text/x-python","patch_set":19,"id":"3a71b18c_1915f2e3","line":145,"range":{"start_line":145,"start_character":36,"end_line":145,"end_character":58},"in_reply_to":"5a74a57a_feb55eec","updated":"2016-12-06 18:03:13.000000000","message":"You\u0027re right that it changes the behaviour of swap on Lvm. However, I think it actually fixes it :) This causes _create_swap to write directly to the newly created Lvm disk, the same as ephemeral disks. This is almost certainly what was intended[1]. It\u0027s also backwards compatible.\n\n[1] Incidentally, it\u0027s also what we need to do with all the other backends, because using locally created filesystems for backing files is broken for migration, and an over-complex waste of space.","commit_id":"d303fb1412e4e974cd596e32675be8420064930b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"760e00276e3ef26ecf92b1c64bf567974c2f8ca9","unresolved":false,"context_lines":[{"line_number":133,"context_line":"        if source_disk_info is not None:"},{"line_number":134,"context_line":"            backing_file \u003d source_disk_info.get(\u0027backing_file\u0027)"},{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"},{"line_number":139,"context_line":"                cache_name \u003d backing_file"}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_cf8c2582","line":136,"range":{"start_line":136,"start_character":44,"end_line":136,"end_character":53},"updated":"2016-12-13 20:26:33.000000000","message":"This should be %(remote)s","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"33699766851dc30a445e89d0959ad54ca5266db5","unresolved":false,"context_lines":[{"line_number":133,"context_line":"        if source_disk_info is not None:"},{"line_number":134,"context_line":"            backing_file \u003d source_disk_info.get(\u0027backing_file\u0027)"},{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"},{"line_number":139,"context_line":"                cache_name \u003d backing_file"}],"source_content_type":"text/x-python","patch_set":23,"id":"ba5201f7_70ae4ce7","line":136,"range":{"start_line":136,"start_character":44,"end_line":136,"end_character":53},"in_reply_to":"1a6eadb0_cf8c2582","updated":"2017-01-05 16:09:17.000000000","message":"Done","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"760e00276e3ef26ecf92b1c64bf567974c2f8ca9","unresolved":false,"context_lines":[{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"},{"line_number":139,"context_line":"                cache_name \u003d backing_file"},{"line_number":140,"context_line":""},{"line_number":141,"context_line":"        # We pass in an ephemeral size for compatibility with the Lvm"}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_af9449e2","line":138,"range":{"start_line":138,"start_character":26,"end_line":138,"end_character":65},"updated":"2016-12-13 20:26:33.000000000","message":"This has to be a dict. Either dict(remote\u003dbacking_file, default\u003dcache_name) or {\u0027remote\u0027: backing_file, \u0027default\u0027: cache_name}","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"33699766851dc30a445e89d0959ad54ca5266db5","unresolved":false,"context_lines":[{"line_number":135,"context_line":"            if backing_file is not None and backing_file !\u003d cache_name:"},{"line_number":136,"context_line":"                LOG.debug(\u0027Using cache_name %(remote) from remote disk \u0027"},{"line_number":137,"context_line":"                          \u0027overriding default %(default)s\u0027,"},{"line_number":138,"context_line":"                          remote\u003dbacking_file, default\u003dcache_name)"},{"line_number":139,"context_line":"                cache_name \u003d backing_file"},{"line_number":140,"context_line":""},{"line_number":141,"context_line":"        # We pass in an ephemeral size for compatibility with the Lvm"}],"source_content_type":"text/x-python","patch_set":23,"id":"ba5201f7_d038d8a2","line":138,"range":{"start_line":138,"start_character":26,"end_line":138,"end_character":65},"in_reply_to":"1a6eadb0_af9449e2","updated":"2017-01-05 16:09:17.000000000","message":"I think I had a fight with a HACKING rule about this at some point in the history of this series. They\u0027ve all been switched and switched back variously. Must have missed this.","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"caf9a8013b8be30db63e65fedfe589f47c4ef1c4","unresolved":false,"context_lines":[{"line_number":190,"context_line":"            def copy_from_host(target, *args, **kwargs):"},{"line_number":191,"context_line":"                libvirt_utils.copy_image(src\u003dtarget, dest\u003dtarget,"},{"line_number":192,"context_line":"                                         host\u003dfallback, receive\u003dTrue)"},{"line_number":193,"context_line":"            self.cache(copy_from_host, cache_name)"},{"line_number":194,"context_line":""},{"line_number":195,"context_line":"    @abc.abstractmethod"},{"line_number":196,"context_line":"    def resize_image(self, size):"}],"source_content_type":"text/x-python","patch_set":23,"id":"3a71b18c_914404b8","line":193,"updated":"2016-12-08 21:14:55.000000000","message":"any reason not to supply size kwarg as on line 180?","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"53fe8660aac86b6980f91315c5f73ea8b71fc0e0","unresolved":false,"context_lines":[{"line_number":190,"context_line":"            def copy_from_host(target, *args, **kwargs):"},{"line_number":191,"context_line":"                libvirt_utils.copy_image(src\u003dtarget, dest\u003dtarget,"},{"line_number":192,"context_line":"                                         host\u003dfallback, receive\u003dTrue)"},{"line_number":193,"context_line":"            self.cache(copy_from_host, cache_name)"},{"line_number":194,"context_line":""},{"line_number":195,"context_line":"    @abc.abstractmethod"},{"line_number":196,"context_line":"    def resize_image(self, size):"}],"source_content_type":"text/x-python","patch_set":23,"id":"3a71b18c_95e93a1d","line":193,"in_reply_to":"3a71b18c_914404b8","updated":"2016-12-09 10:06:26.000000000","message":"This code is copied from the former _try_fetch_image_cache, which didn\u0027t pass size here. I\u0027m going to have to spend some time convincing myself that wasn\u0027t a bug.","commit_id":"c5bdd70ca5918a1e6ae006d1f5122bd48284a734"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"13725db92667e47ec4bd74e9050f097cc11d80c1","unresolved":false,"context_lines":[{"line_number":123,"context_line":"        :param source_disk_info: instance_disk_info from a remote host. We"},{"line_number":124,"context_line":"                                 will copy parameters from this data when"},{"line_number":125,"context_line":"                                 creating the disk locally."},{"line_number":126,"context_line":"        :param fallback: A compute host to side-load the output from if it"},{"line_number":127,"context_line":"                         is not already in the image cache."},{"line_number":128,"context_line":"        \"\"\""},{"line_number":129,"context_line":"        # Define a compatibility function which will ignore any arguments"}],"source_content_type":"text/x-python","patch_set":25,"id":"ba5201f7_34416d0c","line":126,"updated":"2017-01-10 17:15:38.000000000","message":"I doubt that we need to copy the backing file from the source node. You want to keep the same filesystem id, but if a cache file with the same name is already created on the host (for another instance), we\u0027ll use that file despite its obviously different filesystem id. This means that this case must work well. But if it does, we may just create the file from the function if the file does not exist. We\u0027ll get the same result in this case. If it works, why do we need an alternative way to create the file on the host?","commit_id":"1bbcb30e4175fc5624fdb6dc32049c84706194a9"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"13725db92667e47ec4bd74e9050f097cc11d80c1","unresolved":false,"context_lines":[{"line_number":161,"context_line":""},{"line_number":162,"context_line":"        if source_disk_info is not None:"},{"line_number":163,"context_line":"            backing_file \u003d source_disk_info.get(\u0027backing_file\u0027)"},{"line_number":164,"context_line":"            if backing_file is not None:"},{"line_number":165,"context_line":"                cache_name \u003d backing_file"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"        if self.SUPPORTS_CLONE:"}],"source_content_type":"text/x-python","patch_set":25,"id":"ba5201f7_942059b2","line":164,"updated":"2017-01-10 17:15:38.000000000","message":"As we can see in get_instance_disk_info, backing_file value is always not None: https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L7006","commit_id":"1bbcb30e4175fc5624fdb6dc32049c84706194a9"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"32d115027d9fdff1d1090cf54eaca791b8996665","unresolved":false,"context_lines":[{"line_number":190,"context_line":"            def copy_from_host(target, *args, **kwargs):"},{"line_number":191,"context_line":"                libvirt_utils.copy_image(src\u003dtarget, dest\u003dtarget,"},{"line_number":192,"context_line":"                                         host\u003dfallback, receive\u003dTrue)"},{"line_number":193,"context_line":"            self.cache(copy_from_host, cache_name)"},{"line_number":194,"context_line":""},{"line_number":195,"context_line":"    @abc.abstractmethod"},{"line_number":196,"context_line":"    def resize_image(self, size):"}],"source_content_type":"text/x-python","patch_set":25,"id":"ba5201f7_007184a8","line":193,"updated":"2017-01-11 08:45:32.000000000","message":"Interesting... We do not pass image_id here, so Flat backend will copy the disk twice for cold migration. One time in migrate_disk_and_power_off, and another one here inside cache method.","commit_id":"1bbcb30e4175fc5624fdb6dc32049c84706194a9"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"0ee8b79e5bbf46f96e0e156ba6c2b3143f09dbc2","unresolved":false,"context_lines":[{"line_number":806,"context_line":"                    _(\"Instance disk to be encrypted but no context provided\"))"},{"line_number":807,"context_line":"        # Generate images with specified size right on volume"},{"line_number":808,"context_line":"        if generated and size:"},{"line_number":809,"context_line":"            lvm.create_volume(self.vg, self.lv,"},{"line_number":810,"context_line":"                                         size, sparse\u003dself.sparse)"},{"line_number":811,"context_line":"            with self.remove_volume_on_error(self.path):"},{"line_number":812,"context_line":"                if self.ephemeral_key_uuid is not None:"},{"line_number":813,"context_line":"                    encrypt_lvm_image()"},{"line_number":814,"context_line":"                prepare_template(target\u003dself.path, *args, **kwargs)"},{"line_number":815,"context_line":"        else:"},{"line_number":816,"context_line":"            if not os.path.exists(base):"},{"line_number":817,"context_line":"                prepare_template(target\u003dbase, *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":25,"id":"ba5201f7_675a3b2c","line":814,"range":{"start_line":809,"start_character":0,"end_line":814,"end_character":67},"updated":"2017-01-11 13:46:25.000000000","message":"Now this leads to a fault for swap. In this case generated\u003d\u003dTrue (since you always pass ephemeral_size\u003d\u0027dummy\u0027), and prepare_template is called with target pointed to the created volume. Since [1] it calls _create_swap which creates raw image there on target path with no any condition.\n\nSo it makes sense to put your [2] at the first place on the series. And fix there Lvm\u0027s create_image to manage swap here in the \u0027generated\u0027 branch.\n\nOr hope that [2] will be merged soon...\n\n[1] https://review.openstack.org/#/c/408152/\n[2] https://review.openstack.org/#/c/333263/","commit_id":"1bbcb30e4175fc5624fdb6dc32049c84706194a9"}]}