)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"145cb21843d90e15cadd8f85aa4327b9ebed644b","unresolved":false,"context_lines":[{"line_number":17,"context_line":""},{"line_number":18,"context_line":"manager.py: _do_build_and_run_instance()"},{"line_number":19,"context_line":"             .."},{"line_number":20,"context_line":"               _default_block_device_names() \u003c-here"},{"line_number":21,"context_line":"               .."},{"line_number":22,"context_line":"               driver.spawn()"},{"line_number":23,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"3fa7e38b_fba1253b","line":20,"updated":"2019-11-25 15:34:37.000000000","message":"There are some other weird edge cases like the bdm v1 ec2/s3 stuff still supported in the API and some xenapi driver edge cases I think but those are both non-standard so not really worth worrying about here probably.\n\nhttps://github.com/openstack/nova/blob/78c5577a5c921e851df3fc0efd8c03de790a0b9e/nova/block_device.py#L374\n\nhttps://github.com/openstack/nova/blob/78c5577a5c921e851df3fc0efd8c03de790a0b9e/nova/block_device.py#L543\n\nAnd it also looks like when creating a snapshot of a volume-backed instance we\u0027ll set that in the snapshot image bdm metadata so when you create a server from that snapshot we\u0027ll create a volume-backed server and use that root device name:\n\nhttps://github.com/openstack/nova/blob/78c5577a5c921e851df3fc0efd8c03de790a0b9e/nova/compute/api.py#L3172","commit_id":"5e0ed5e7fee3c4c887263a0e9fa847c2dcc5cf3b"}],"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":"145cb21843d90e15cadd8f85aa4327b9ebed644b","unresolved":false,"context_lines":[{"line_number":21945,"context_line":"                     mock_build_device_metadata, mock_set_host_enabled,"},{"line_number":21946,"context_line":"                     mock_write_to_file,"},{"line_number":21947,"context_line":"                     mock_get_mdev,"},{"line_number":21948,"context_line":"                     image_meta_dict\u003dNone,"},{"line_number":21949,"context_line":"                     exists\u003dNone):"},{"line_number":21950,"context_line":"        self.flags(instances_path\u003dself.useFixture(fixtures.TempDir()).path)"},{"line_number":21951,"context_line":"        mock_build_device_metadata.return_value \u003d None"}],"source_content_type":"text/x-python","patch_set":4,"id":"3fa7e38b_2636c4e8","line":21948,"updated":"2019-11-25 15:34:37.000000000","message":"nit: normally I\u0027d add new kwargs to the end of the signature.","commit_id":"5e0ed5e7fee3c4c887263a0e9fa847c2dcc5cf3b"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"9716f9c0ce28694726889b914dbeb85c8961a26f","unresolved":false,"context_lines":[{"line_number":5348,"context_line":"        else:"},{"line_number":5349,"context_line":"            root_device_name \u003d None"},{"line_number":5350,"context_line":""},{"line_number":5351,"context_line":"        if root_device_name and not instance.get(\u0027root_device_name\u0027, None):"},{"line_number":5352,"context_line":"            instance.root_device_name \u003d root_device_name"},{"line_number":5353,"context_line":""},{"line_number":5354,"context_line":"        guest.os_type \u003d (fields.VMMode.get_from_instance(instance) or"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_28fcee3d","line":5351,"updated":"2019-11-13 14:14:55.000000000","message":"I was going to say the `, None` is redundant. But then I remembered we\u0027re dealing with an OVO, which means I don\u0027t have a clue.\n\nSince root_device_name is nullable [1], isn\u0027t this one of those situations where we have to do hasattr/in first?\n\n[1] https://opendev.org/openstack/nova/src/commit/7aa88029bbf6311033457c32801963da01e88ecb/nova/objects/instance.py#L175","commit_id":"e7e5102ec3edaedf6b5336997e246295c0a5b732"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"0dd651f753d759d776c6fe4541d979b5ffd70d72","unresolved":false,"context_lines":[{"line_number":5348,"context_line":"        else:"},{"line_number":5349,"context_line":"            root_device_name \u003d None"},{"line_number":5350,"context_line":""},{"line_number":5351,"context_line":"        if root_device_name and not instance.get(\u0027root_device_name\u0027, None):"},{"line_number":5352,"context_line":"            instance.root_device_name \u003d root_device_name"},{"line_number":5353,"context_line":""},{"line_number":5354,"context_line":"        guest.os_type \u003d (fields.VMMode.get_from_instance(instance) or"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_fe3bec35","line":5351,"in_reply_to":"3fa7e38b_28fcee3d","updated":"2019-11-13 15:29:58.000000000","message":"\u003e I was going to say the `, None` is redundant. But then I remembered\n \u003e we\u0027re dealing with an OVO, which means I don\u0027t have a clue.\n\nMaybe not...\n\n \u003e Since root_device_name is nullable [1], isn\u0027t this one of those\n \u003e situations where we have to do hasattr/in first?\n\nI initially thought the same, but this is relying on \u0027base.NovaObjectDictCompat\u0027 mixin on the \u0027Instance\u0027 object. However, since we\u0027re using that, doesn\u0027t \u0027get\u0027 behave like the dict implementation. Specifically, shouldn\u0027t \u0027None\u0027 be unnecessary as efried notes above.\n\n \u003e [1] https://opendev.org/openstack/nova/src/commit/7aa88029bbf6311033457c32801963da01e88ecb/nova/objects/instance.py#L175","commit_id":"e7e5102ec3edaedf6b5336997e246295c0a5b732"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"55378b4ddb4b08969f5785fb1b0f3695c0b1164f","unresolved":false,"context_lines":[{"line_number":5348,"context_line":"        else:"},{"line_number":5349,"context_line":"            root_device_name \u003d None"},{"line_number":5350,"context_line":""},{"line_number":5351,"context_line":"        if root_device_name and not instance.get(\u0027root_device_name\u0027, None):"},{"line_number":5352,"context_line":"            instance.root_device_name \u003d root_device_name"},{"line_number":5353,"context_line":""},{"line_number":5354,"context_line":"        guest.os_type \u003d (fields.VMMode.get_from_instance(instance) or"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_1ef128df","line":5351,"in_reply_to":"3fa7e38b_28fcee3d","updated":"2019-11-13 15:43:39.000000000","message":"The commit message says that the goal is to avoid updating the instance root_device_name from the rescue image. I\u0027m not sure how this does that, except maybe something specific to your rescue image, config or environment.","commit_id":"e7e5102ec3edaedf6b5336997e246295c0a5b732"},{"author":{"_account_id":28332,"name":"Alexandre arents","email":"alexandre.arents@corp.ovh.com","username":"aarents"},"change_message_id":"aefa456f9b38180618c67729c3d8d7b26392bdf5","unresolved":false,"context_lines":[{"line_number":5348,"context_line":"        else:"},{"line_number":5349,"context_line":"            root_device_name \u003d None"},{"line_number":5350,"context_line":""},{"line_number":5351,"context_line":"        if root_device_name and not instance.get(\u0027root_device_name\u0027, None):"},{"line_number":5352,"context_line":"            instance.root_device_name \u003d root_device_name"},{"line_number":5353,"context_line":""},{"line_number":5354,"context_line":"        guest.os_type \u003d (fields.VMMode.get_from_instance(instance) or"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_903e55b7","line":5351,"in_reply_to":"3fa7e38b_391f5e5c","updated":"2019-11-14 10:49:39.000000000","message":"I must confess that having  _get_stuf() updating stuff disappointing me too.\n\nI fully agree that removing this line seems the best solution, that was my first intention BUT doing this break this unitary test:\n   nova.tests.unit.virt.libvirt.test_driver.LibvirtConnTestCase.test_get_guest_config_bug_1118829\n\nintroduced here: https://review.opendev.org/#/c/21473\n\nerror:\nTraceback (most recent call last):\n   File \"nova/tests/unit/virt/libvirt/test_driver.py\", line 4549, in test_get_guest_config_bug_1118829\n   self.assertEqual(instance_ref[\u0027root_device_name\u0027], \u0027/dev/vda\u0027)\n\nI\u0027m wondering if this test (from 2013)  is still relevant with the current code? I\u0027m not sure at all, can someone confirm ?\n\nIf not I propose to remove both this line and test_get_guest_config_bug_1118829 (tests are ok after that)","commit_id":"e7e5102ec3edaedf6b5336997e246295c0a5b732"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9082b98368d72436f83360bda80359d1564b8448","unresolved":false,"context_lines":[{"line_number":5348,"context_line":"        else:"},{"line_number":5349,"context_line":"            root_device_name \u003d None"},{"line_number":5350,"context_line":""},{"line_number":5351,"context_line":"        if root_device_name and not instance.get(\u0027root_device_name\u0027, None):"},{"line_number":5352,"context_line":"            instance.root_device_name \u003d root_device_name"},{"line_number":5353,"context_line":""},{"line_number":5354,"context_line":"        guest.os_type \u003d (fields.VMMode.get_from_instance(instance) or"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_fc84420d","line":5351,"in_reply_to":"3fa7e38b_903e55b7","updated":"2019-11-14 14:46:57.000000000","message":"Right, the test is there to validate the code, not prove that it\u0027s necessary or a good idea. If something else breaks, then that\u0027s something to discuss but just the unit test that validates that it\u0027s doing what we know it\u0027s doing is obviously going to have to change.","commit_id":"e7e5102ec3edaedf6b5336997e246295c0a5b732"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"55378b4ddb4b08969f5785fb1b0f3695c0b1164f","unresolved":false,"context_lines":[{"line_number":5348,"context_line":"        else:"},{"line_number":5349,"context_line":"            root_device_name \u003d None"},{"line_number":5350,"context_line":""},{"line_number":5351,"context_line":"        if root_device_name and not instance.get(\u0027root_device_name\u0027, None):"},{"line_number":5352,"context_line":"            instance.root_device_name \u003d root_device_name"},{"line_number":5353,"context_line":""},{"line_number":5354,"context_line":"        guest.os_type \u003d (fields.VMMode.get_from_instance(instance) or"}],"source_content_type":"text/x-python","patch_set":1,"id":"3fa7e38b_391f5e5c","line":5351,"in_reply_to":"3fa7e38b_fe3bec35","updated":"2019-11-13 15:43:39.000000000","message":"obj.get(key, None) avoids a lazy-load, so there\u0027s a reason why we do that in places.\n\nHowever, I think we should probably remove this line entirely. When it was originally added, it was a direct-to-db update call. When I converted all of those to object actions, there was an \"instance.save()\" below to immediately update the device name in the database. That was removed in a sweeping attempt to remove a bunch of extra instance.save() calls here:\n\nhttps://review.opendev.org/#/c/119622\n\nI think that the author and reviewers of that probably missed the nuance that _get_guest_config() is called in many places other than just during instance boot.\n\nThat said, I think that (a) it\u0027s pretty bad to put something that modifies the instance at all in a \"get xml\" method that a caller would have every expectation did not have any side-effects and (b) it probably has some very weird behavior now, dirtying the instance but only getting persisted if that instance object happens to be .save()d somewhere after this.\n\nAs far as I can tell we always set instance.root_device_name from the build_resources() chain during spawn() in compute manager (at least for libvirt, which is what this is for). I would argue that we should probably just remove this obscure side effect instead of making it more obscure in an attempt to whack-a-mole the symptom experienced in the bug.\n\nDoes anyone know of a reason *not* to just remove this?","commit_id":"e7e5102ec3edaedf6b5336997e246295c0a5b732"}]}