)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3065ff9b139e31018c71d1038ca8f1e7c1c250d4","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Up until now, libvirt would default it\u0027s own device names unless they"},{"line_number":10,"context_line":"were specified by the user at boot. This could lead to inconsistencies,"},{"line_number":11,"context_line":"so the soft approach was to discourage users from passing in explicit"},{"line_number":12,"context_line":"device names, but not override them."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"It turns out that this approach causes more trouble, especially when we"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"fa32b979_4b7c8db5","line":11,"updated":"2015-06-25 19:59:05.000000000","message":"Where was this discouraged?  Docs?  Logs?  Red Hat support people?  I\u0027m trying to build some historical context to wrap my head around this.","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f20f68f13131befbfb3e2f500bbb7d62c195f053","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Up until now, libvirt would default it\u0027s own device names unless they"},{"line_number":10,"context_line":"were specified by the user at boot. This could lead to inconsistencies,"},{"line_number":11,"context_line":"so the soft approach was to discourage users from passing in explicit"},{"line_number":12,"context_line":"device names, but not override them."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"It turns out that this approach causes more trouble, especially when we"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"fa32b979_466c5036","line":11,"in_reply_to":"fa32b979_2d3ee8da","updated":"2015-06-26 19:28:19.000000000","message":"Yeah, so a bug with some tribal knowledge but never actually documented anywhere is what I\u0027m getting from this.  Which is fine as an answer, I\u0027m just wondering.","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"adadd4ca9e002669c50a0c844be2bf083e5709f3","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Up until now, libvirt would default it\u0027s own device names unless they"},{"line_number":10,"context_line":"were specified by the user at boot. This could lead to inconsistencies,"},{"line_number":11,"context_line":"so the soft approach was to discourage users from passing in explicit"},{"line_number":12,"context_line":"device names, but not override them."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"It turns out that this approach causes more trouble, especially when we"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"ba3cc151_dfe27b9f","line":11,"in_reply_to":"fa32b979_466c5036","updated":"2015-06-29 17:06:45.000000000","message":"Documented here though not merged yet: https://review.openstack.org/#/c/192651/","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"de9595ec2e9a3e09cb32f3a0c51c2e1bf90247f4","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Up until now, libvirt would default it\u0027s own device names unless they"},{"line_number":10,"context_line":"were specified by the user at boot. This could lead to inconsistencies,"},{"line_number":11,"context_line":"so the soft approach was to discourage users from passing in explicit"},{"line_number":12,"context_line":"device names, but not override them."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"It turns out that this approach causes more trouble, especially when we"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"fa32b979_2d3ee8da","line":11,"in_reply_to":"fa32b979_4b7c8db5","updated":"2015-06-26 09:27:43.000000000","message":"It was discouraged in the sense that if the user passed in a device name it would be ignored by libvirt and/or guest OS - so they\u0027d have to find a different way regardless :-)","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ff42bb0a9618568a1e65f7b368ab7c1d417ddd01","unresolved":false,"context_lines":[{"line_number":23,"context_line":"before we switch attach_volume over too, so that there is consistent"},{"line_number":24,"context_line":"behavior."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"Also - make tests for defaulting device names more realistic as they now"},{"line_number":27,"context_line":"actually run the code instead of just making sure the right methods get"},{"line_number":28,"context_line":"called."},{"line_number":29,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"ba3cc151_b45b0a2e","line":26,"updated":"2015-07-06 15:50:54.000000000","message":"They are more realistic but also now more brittle since they will be tightly coupled to the blockinfo code, so if that changes and impacts the tests in the libvirt driver we also have to update there, which kind of sucks.  And it makes it harder for those not intimately familiar with the blockinfo code to understand how the inputs in the driver test are changed and result in the expected outputs (via the asserted device_name changes on the bdm fakes).\n\nMaybe some comments in the test about why we expect the device_names to change for the swap and volumes bdms but not the ephemeral bdm would be helpful.","commit_id":"907de5b487bc2b6a9ea50f8a9f7d3f9bac744db4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2446bd1829fdb14cfed4f0bee25a382ec9df39de","unresolved":false,"context_lines":[{"line_number":23,"context_line":"before we switch attach_volume over too, so that there is consistent"},{"line_number":24,"context_line":"behavior."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"Also - make tests for defaulting device names more realistic as they now"},{"line_number":27,"context_line":"actually run the code instead of just making sure the right methods get"},{"line_number":28,"context_line":"called."},{"line_number":29,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"ba3cc151_0fb5ca2f","line":26,"in_reply_to":"ba3cc151_94600d37","updated":"2015-07-07 15:52:23.000000000","message":"The blockinfo code is really utility code so it shouldn\u0027t change drastically, so yeah, I\u0027m fine with testing through to that level, but comments would be awesome since it\u0027s a confusing path to follow.","commit_id":"907de5b487bc2b6a9ea50f8a9f7d3f9bac744db4"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"976a6f46d10212fa503626b14f0220c62c08d3bd","unresolved":false,"context_lines":[{"line_number":23,"context_line":"before we switch attach_volume over too, so that there is consistent"},{"line_number":24,"context_line":"behavior."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"Also - make tests for defaulting device names more realistic as they now"},{"line_number":27,"context_line":"actually run the code instead of just making sure the right methods get"},{"line_number":28,"context_line":"called."},{"line_number":29,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"ba3cc151_94600d37","line":26,"in_reply_to":"ba3cc151_b45b0a2e","updated":"2015-07-07 15:47:14.000000000","message":"Yeah - so it\u0027s always a judgement call where to mock. I think that in this case it really makes sense to not mock what is esentailly also part of the libvirt driver code.\n\nWhile we don\u0027t want to make tests too brittle - we do want to test the driver functinality, and one could argue that changing this code does in fact require the person to understand how blockinfo works.\n\nBut comments are good so I will surely add some.","commit_id":"907de5b487bc2b6a9ea50f8a9f7d3f9bac744db4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c8f16bcf6265195ee2426418f3feebffb2f31f40","unresolved":false,"context_lines":[{"line_number":27,"context_line":"actually run the code instead of just making sure the right methods get"},{"line_number":28,"context_line":"called."},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"Upgrade-impact: After this commit, supplying a device name for any of"},{"line_number":31,"context_line":"the block devices specified as part of the `nova boot` call will not be"},{"line_number":32,"context_line":"honoured by any libvirt compute nodes."},{"line_number":33,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"ba3cc151_b23241f1","line":30,"updated":"2015-07-10 18:57:14.000000000","message":"The tag is UpgradeImpact, this is important to get right since we scan these at the end of the release looking for things to put in the release notes.  See:\n\nhttps://wiki.openstack.org/wiki/GitCommitMessages#Including_external_references","commit_id":"9d2ebbcb2b37d27403e6d994e496ddd75a8a8f0e"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ff42bb0a9618568a1e65f7b368ab7c1d417ddd01","unresolved":false,"context_lines":[{"line_number":10442,"context_line":"                                               instance.root_device_name,"},{"line_number":10443,"context_line":"                                               ephemerals, swap,"},{"line_number":10444,"context_line":"                                               block_device_mapping,"},{"line_number":10445,"context_line":"                                               image_meta)"},{"line_number":10446,"context_line":"        mock_devnames.assert_called_once_with("},{"line_number":10447,"context_line":"            \"fake_libvirt_type\", mock.ANY,"},{"line_number":10448,"context_line":"            instance, \u0027fake-block-device-info\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_0ee509a7","side":"PARENT","line":10445,"updated":"2015-07-06 15:50:54.000000000","message":"Weird, the method doesn\u0027t even take image_meta in it\u0027s signature so this must have just been wrong before since it\u0027s not part of the bdm list.","commit_id":"b6733dbdf3d04f4e334f1aeb6ee7ce4b6d1b7b95"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ff42bb0a9618568a1e65f7b368ab7c1d417ddd01","unresolved":false,"context_lines":[{"line_number":10423,"context_line":""},{"line_number":10424,"context_line":"    @mock.patch.object(utils, \"get_image_from_system_metadata\")"},{"line_number":10425,"context_line":"    @mock.patch.object(objects.BlockDeviceMapping, \"save\")"},{"line_number":10426,"context_line":"    def test_default_device_names_for_instance(self, save_mock, mock_meta):"},{"line_number":10427,"context_line":"        instance \u003d objects.Instance(**self.test_instance)"},{"line_number":10428,"context_line":"        image_meta \u003d {}"},{"line_number":10429,"context_line":"        instance.root_device_name \u003d \u0027/dev/vda\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_ce496190","line":10426,"updated":"2015-07-06 15:50:54.000000000","message":"nit: could/should probably assert that save_mock was called 3 times.","commit_id":"907de5b487bc2b6a9ea50f8a9f7d3f9bac744db4"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"976a6f46d10212fa503626b14f0220c62c08d3bd","unresolved":false,"context_lines":[{"line_number":10423,"context_line":""},{"line_number":10424,"context_line":"    @mock.patch.object(utils, \"get_image_from_system_metadata\")"},{"line_number":10425,"context_line":"    @mock.patch.object(objects.BlockDeviceMapping, \"save\")"},{"line_number":10426,"context_line":"    def test_default_device_names_for_instance(self, save_mock, mock_meta):"},{"line_number":10427,"context_line":"        instance \u003d objects.Instance(**self.test_instance)"},{"line_number":10428,"context_line":"        image_meta \u003d {}"},{"line_number":10429,"context_line":"        instance.root_device_name \u003d \u0027/dev/vda\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_14dcfde2","line":10426,"in_reply_to":"ba3cc151_ce496190","updated":"2015-07-07 15:47:14.000000000","message":"Done","commit_id":"907de5b487bc2b6a9ea50f8a9f7d3f9bac744db4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ff42bb0a9618568a1e65f7b368ab7c1d417ddd01","unresolved":false,"context_lines":[{"line_number":10458,"context_line":"                                               block_device_mapping)"},{"line_number":10459,"context_line":""},{"line_number":10460,"context_line":"        self.assertEqual(\u0027/dev/vdb\u0027, ephemerals[0].device_name)"},{"line_number":10461,"context_line":"        self.assertEqual(\u0027/dev/vdc\u0027, swap[0].device_name)"},{"line_number":10462,"context_line":"        self.assertEqual(\u0027/dev/sda\u0027, block_device_mapping[0].device_name)"},{"line_number":10463,"context_line":""},{"line_number":10464,"context_line":"    def test_is_supported_fs_format(self):"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_b15ebc74","line":10461,"updated":"2015-07-06 15:50:54.000000000","message":"I\u0027m honestly confused as to where/how the device_names are changed down in blockinfo.get_disk_mapping for the swap and volume bdms given self.test_instance and the bdm values above, unless there is some other fixture / mocking going on.\n\nSince this test is now coupled with the guts of blockinfo, can you put a comment above the assertions explaining why we expect the swap and volume bdm device_names to change but not the ephemeral bdm (besides just adding the /dev/ prefix to it)?","commit_id":"907de5b487bc2b6a9ea50f8a9f7d3f9bac744db4"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"976a6f46d10212fa503626b14f0220c62c08d3bd","unresolved":false,"context_lines":[{"line_number":10458,"context_line":"                                               block_device_mapping)"},{"line_number":10459,"context_line":""},{"line_number":10460,"context_line":"        self.assertEqual(\u0027/dev/vdb\u0027, ephemerals[0].device_name)"},{"line_number":10461,"context_line":"        self.assertEqual(\u0027/dev/vdc\u0027, swap[0].device_name)"},{"line_number":10462,"context_line":"        self.assertEqual(\u0027/dev/sda\u0027, block_device_mapping[0].device_name)"},{"line_number":10463,"context_line":""},{"line_number":10464,"context_line":"    def test_is_supported_fs_format(self):"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_94956de5","line":10461,"in_reply_to":"ba3cc151_b15ebc74","updated":"2015-07-07 15:47:14.000000000","message":"Will add comments - see my response on the commit message regarding the coupling.","commit_id":"907de5b487bc2b6a9ea50f8a9f7d3f9bac744db4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c8f16bcf6265195ee2426418f3feebffb2f31f40","unresolved":false,"context_lines":[{"line_number":10459,"context_line":""},{"line_number":10460,"context_line":"        # Ephemeral device name was correct so no changes"},{"line_number":10461,"context_line":"        self.assertEqual(\u0027/dev/vdb\u0027, ephemerals[0].device_name)"},{"line_number":10462,"context_line":"        # Swap device name was changed"},{"line_number":10463,"context_line":"        self.assertEqual(\u0027/dev/vdc\u0027, swap[0].device_name)"},{"line_number":10464,"context_line":"        # Volum device name was changed, taking the bus into account"},{"line_number":10465,"context_line":"        self.assertEqual(\u0027/dev/sda\u0027, block_device_mapping[0].device_name)"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_129f6d51","line":10462,"updated":"2015-07-10 18:57:14.000000000","message":"Changed why?  I like how you have \", taking the bus into account\" for the volume case, so if you can add something like that for swap that\u0027s goodness.","commit_id":"9d2ebbcb2b37d27403e6d994e496ddd75a8a8f0e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c8f16bcf6265195ee2426418f3feebffb2f31f40","unresolved":false,"context_lines":[{"line_number":10461,"context_line":"        self.assertEqual(\u0027/dev/vdb\u0027, ephemerals[0].device_name)"},{"line_number":10462,"context_line":"        # Swap device name was changed"},{"line_number":10463,"context_line":"        self.assertEqual(\u0027/dev/vdc\u0027, swap[0].device_name)"},{"line_number":10464,"context_line":"        # Volum device name was changed, taking the bus into account"},{"line_number":10465,"context_line":"        self.assertEqual(\u0027/dev/sda\u0027, block_device_mapping[0].device_name)"},{"line_number":10466,"context_line":""},{"line_number":10467,"context_line":"        self.assertEqual(3, save_mock.call_count)"}],"source_content_type":"text/x-python","patch_set":7,"id":"ba3cc151_d2a8257a","line":10464,"updated":"2015-07-10 18:57:14.000000000","message":"nit: s/Volum/Volume/","commit_id":"9d2ebbcb2b37d27403e6d994e496ddd75a8a8f0e"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3065ff9b139e31018c71d1038ca8f1e7c1c250d4","unresolved":false,"context_lines":[{"line_number":6817,"context_line":"            instance.system_metadata)"},{"line_number":6818,"context_line":""},{"line_number":6819,"context_line":"        block_device_mapping \u003d list(itertools.chain(*block_device_lists))"},{"line_number":6820,"context_line":"        # NOTE(ndipanov): Null out the device names so that blockinfo code"},{"line_number":6821,"context_line":"        #                 will assign them"},{"line_number":6822,"context_line":"        for bdm in block_device_mapping:"},{"line_number":6823,"context_line":"            if bdm.device_name is not None:"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa32b979_abb3d999","line":6820,"updated":"2015-06-25 19:59:05.000000000","message":"So, does this void the compute API contract where device_name could be passed in on the bdm dict?\n\nhttp://git.openstack.org/cgit/openstack/nova/tree/nova/api/openstack/compute/schemas/v3/block_device_mapping_v1.py#n28\n\nhttp://git.openstack.org/cgit/openstack/nova/tree/nova/api/openstack/compute/servers.py#n405\n\nI guess that\u0027s the intent.","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"adadd4ca9e002669c50a0c844be2bf083e5709f3","unresolved":false,"context_lines":[{"line_number":6817,"context_line":"            instance.system_metadata)"},{"line_number":6818,"context_line":""},{"line_number":6819,"context_line":"        block_device_mapping \u003d list(itertools.chain(*block_device_lists))"},{"line_number":6820,"context_line":"        # NOTE(ndipanov): Null out the device names so that blockinfo code"},{"line_number":6821,"context_line":"        #                 will assign them"},{"line_number":6822,"context_line":"        for bdm in block_device_mapping:"},{"line_number":6823,"context_line":"            if bdm.device_name is not None:"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_3f0cc793","line":6820,"in_reply_to":"fa32b979_abb3d999","updated":"2015-06-29 17:06:45.000000000","message":"yes that is pretty much the sole intent of this patch","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3065ff9b139e31018c71d1038ca8f1e7c1c250d4","unresolved":false,"context_lines":[{"line_number":6821,"context_line":"        #                 will assign them"},{"line_number":6822,"context_line":"        for bdm in block_device_mapping:"},{"line_number":6823,"context_line":"            if bdm.device_name is not None:"},{"line_number":6824,"context_line":"                LOG.warn(_LW(\u0027Ignoring supplied device name: %(device_name)s\u0027),"},{"line_number":6825,"context_line":"                     {\u0027device_name\u0027: bdm.device_name}, instance\u003dinstance)"},{"line_number":6826,"context_line":"                bdm.device_name \u003d None"},{"line_number":6827,"context_line":"        block_device_info \u003d driver.get_block_device_info(instance,"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa32b979_0b668529","line":6824,"updated":"2015-06-25 19:59:05.000000000","message":"If this is going to be a warning, we should probably explain why it\u0027s being ignored so people stop sending it in - even though only the admin will see it, but if someone says, boot the server from a volume with device name and you don\u0027t give it to me, and I open a bug report, there should be some explanation in the warning log about why it\u0027s ignored.","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"adadd4ca9e002669c50a0c844be2bf083e5709f3","unresolved":false,"context_lines":[{"line_number":6821,"context_line":"        #                 will assign them"},{"line_number":6822,"context_line":"        for bdm in block_device_mapping:"},{"line_number":6823,"context_line":"            if bdm.device_name is not None:"},{"line_number":6824,"context_line":"                LOG.warn(_LW(\u0027Ignoring supplied device name: %(device_name)s\u0027),"},{"line_number":6825,"context_line":"                     {\u0027device_name\u0027: bdm.device_name}, instance\u003dinstance)"},{"line_number":6826,"context_line":"                bdm.device_name \u003d None"},{"line_number":6827,"context_line":"        block_device_info \u003d driver.get_block_device_info(instance,"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_9c6c089e","line":6824,"in_reply_to":"fa32b979_0b668529","updated":"2015-06-29 17:06:45.000000000","message":"Will add a more detailed description - thanks!","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3065ff9b139e31018c71d1038ca8f1e7c1c250d4","unresolved":false,"context_lines":[{"line_number":6822,"context_line":"        for bdm in block_device_mapping:"},{"line_number":6823,"context_line":"            if bdm.device_name is not None:"},{"line_number":6824,"context_line":"                LOG.warn(_LW(\u0027Ignoring supplied device name: %(device_name)s\u0027),"},{"line_number":6825,"context_line":"                     {\u0027device_name\u0027: bdm.device_name}, instance\u003dinstance)"},{"line_number":6826,"context_line":"                bdm.device_name \u003d None"},{"line_number":6827,"context_line":"        block_device_info \u003d driver.get_block_device_info(instance,"},{"line_number":6828,"context_line":"                                                         block_device_mapping)"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa32b979_cb5c1d80","line":6825,"updated":"2015-06-25 19:59:05.000000000","message":"nit: alignment","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3065ff9b139e31018c71d1038ca8f1e7c1c250d4","unresolved":false,"context_lines":[{"line_number":6827,"context_line":"        block_device_info \u003d driver.get_block_device_info(instance,"},{"line_number":6828,"context_line":"                                                         block_device_mapping)"},{"line_number":6829,"context_line":""},{"line_number":6830,"context_line":"        blockinfo.default_device_names(CONF.libvirt.virt_type,"},{"line_number":6831,"context_line":"                                       nova_context.get_admin_context(),"},{"line_number":6832,"context_line":"                                       instance,"},{"line_number":6833,"context_line":"                                       block_device_info,"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa32b979_4b8b2d69","line":6830,"updated":"2015-06-25 19:59:05.000000000","message":"This is unrelated, but why is get_disk_info called in here?\n\nhttp://git.openstack.org/cgit/openstack/nova/tree/nova/virt/libvirt/blockinfo.py#n469\n\nget_disk_info returns a dict that\u0027s not used in default_device_names.\n\nLooks like that\u0027s been around since havana but isn\u0027t used:\n\nf9f247ef42c690bf1bd1ae65135c6e7fbd0d3c8c","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"af678132e52a6a98e897f768c210d5ae96422a39","unresolved":false,"context_lines":[{"line_number":6827,"context_line":"        block_device_info \u003d driver.get_block_device_info(instance,"},{"line_number":6828,"context_line":"                                                         block_device_mapping)"},{"line_number":6829,"context_line":""},{"line_number":6830,"context_line":"        blockinfo.default_device_names(CONF.libvirt.virt_type,"},{"line_number":6831,"context_line":"                                       nova_context.get_admin_context(),"},{"line_number":6832,"context_line":"                                       instance,"},{"line_number":6833,"context_line":"                                       block_device_info,"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_8d2a827d","line":6830,"in_reply_to":"ba3cc151_92305da0","updated":"2015-07-06 14:51:34.000000000","message":"I guess that code path eventually does get down into blockinfo.update_bdm where the device_name is set.  It\u0027s just not obvious from the start that the get_disk_info method is mutating the bdm and setting the device_name eventually.\n\nblockinfo.get_disk_mapping seems scary to me at least. :)","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8bcc589175d042489ef27e891c9a57d35d3af4b8","unresolved":false,"context_lines":[{"line_number":6827,"context_line":"        block_device_info \u003d driver.get_block_device_info(instance,"},{"line_number":6828,"context_line":"                                                         block_device_mapping)"},{"line_number":6829,"context_line":""},{"line_number":6830,"context_line":"        blockinfo.default_device_names(CONF.libvirt.virt_type,"},{"line_number":6831,"context_line":"                                       nova_context.get_admin_context(),"},{"line_number":6832,"context_line":"                                       instance,"},{"line_number":6833,"context_line":"                                       block_device_info,"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_92305da0","line":6830,"in_reply_to":"ba3cc151_9fccf336","updated":"2015-07-06 14:41:27.000000000","message":"Yeah, as you\u0027ve probably noticed by now, I\u0027m not a big fan of pass by reference methods that are named something like \u0027get_*\u0027 because it sounds like I should get something back and then use it, not pass something in and expect it to be changed - in that case I\u0027d rename methods to something like \u0027update_*\u0027 or \u0027modify_*\u0027.  Since this is all Python and a more OO approach, we should be following convention there rather than what\u0027s normal in C, since a lot of people don\u0027t write things in C anymore for a reason. :)","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"adadd4ca9e002669c50a0c844be2bf083e5709f3","unresolved":false,"context_lines":[{"line_number":6827,"context_line":"        block_device_info \u003d driver.get_block_device_info(instance,"},{"line_number":6828,"context_line":"                                                         block_device_mapping)"},{"line_number":6829,"context_line":""},{"line_number":6830,"context_line":"        blockinfo.default_device_names(CONF.libvirt.virt_type,"},{"line_number":6831,"context_line":"                                       nova_context.get_admin_context(),"},{"line_number":6832,"context_line":"                                       instance,"},{"line_number":6833,"context_line":"                                       block_device_info,"}],"source_content_type":"text/x-python","patch_set":2,"id":"ba3cc151_9fccf336","line":6830,"in_reply_to":"fa32b979_4b8b2d69","updated":"2015-06-29 17:06:45.000000000","message":"it is very much related - that method is what will assign device names if they are not set.\n\nDue to the general structure of that code when it was orriginally written, it mutates the structures passed in instead. It\u0027s not as strange as if you were used to passing in pointers to structures in C, and a bit strange if you are used to python\u0027s more OO approach.","commit_id":"7b195b5fa8ea3e10fc1fc53283c05b081668d26e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ff42bb0a9618568a1e65f7b368ab7c1d417ddd01","unresolved":false,"context_lines":[{"line_number":6821,"context_line":"        #                 will assign them"},{"line_number":6822,"context_line":"        for bdm in block_device_mapping:"},{"line_number":6823,"context_line":"            if bdm.device_name is not None:"},{"line_number":6824,"context_line":"                LOG.warn(_LW(\"Ignoring supplied device name: %(device_name)s. \""},{"line_number":6825,"context_line":"                             \"Libvirt can\u0027t honour user-supplied dev names\"),"},{"line_number":6826,"context_line":"                         {\u0027device_name\u0027: bdm.device_name}, instance\u003dinstance)"},{"line_number":6827,"context_line":"                bdm.device_name \u003d None"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_b1d55cba","line":6824,"updated":"2015-07-06 15:50:54.000000000","message":"I kind of wonder if we should have an UpgradeImpact in the commit message so we remember to put something in the Liberty release notes about this, saying basically requested device_names are now going to be ignored when using the libvirt driver.","commit_id":"907de5b487bc2b6a9ea50f8a9f7d3f9bac744db4"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"29c372e51be65a367d66b770b607fab1afd0e15d","unresolved":false,"context_lines":[{"line_number":6821,"context_line":"        #                 will assign them"},{"line_number":6822,"context_line":"        for bdm in block_device_mapping:"},{"line_number":6823,"context_line":"            if bdm.device_name is not None:"},{"line_number":6824,"context_line":"                LOG.warn(_LW(\"Ignoring supplied device name: %(device_name)s. \""},{"line_number":6825,"context_line":"                             \"Libvirt can\u0027t honour user-supplied dev names\"),"},{"line_number":6826,"context_line":"                         {\u0027device_name\u0027: bdm.device_name}, instance\u003dinstance)"},{"line_number":6827,"context_line":"                bdm.device_name \u003d None"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_6599156e","line":6824,"in_reply_to":"ba3cc151_2a8afcbf","updated":"2015-07-07 16:37:03.000000000","message":"So I am not sure this has any API impact - it will not error out if the device name was specified, it will just potentially give it a different value.","commit_id":"907de5b487bc2b6a9ea50f8a9f7d3f9bac744db4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"15082349fad2f0a17f3491804d1beb3903bcd168","unresolved":false,"context_lines":[{"line_number":6821,"context_line":"        #                 will assign them"},{"line_number":6822,"context_line":"        for bdm in block_device_mapping:"},{"line_number":6823,"context_line":"            if bdm.device_name is not None:"},{"line_number":6824,"context_line":"                LOG.warn(_LW(\"Ignoring supplied device name: %(device_name)s. \""},{"line_number":6825,"context_line":"                             \"Libvirt can\u0027t honour user-supplied dev names\"),"},{"line_number":6826,"context_line":"                         {\u0027device_name\u0027: bdm.device_name}, instance\u003dinstance)"},{"line_number":6827,"context_line":"                bdm.device_name \u003d None"}],"source_content_type":"text/x-python","patch_set":6,"id":"ba3cc151_2a8afcbf","line":6824,"in_reply_to":"ba3cc151_b1d55cba","updated":"2015-07-07 16:14:43.000000000","message":"APIImpact is actually probably more appropriate here:\n\nhttp://specs.openstack.org/openstack/api-wg/guidelines/evaluating_api_changes.html\n\nAnd it\u0027s probably OK given \"Changing an error response code to be more accurate.\" is generally acceptable, and it sounds like w/o this there are bugs.","commit_id":"907de5b487bc2b6a9ea50f8a9f7d3f9bac744db4"}]}