)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":20733,"name":"Rajesh Tailor","email":"ratailor@redhat.com","username":"rajesht"},"change_message_id":"0caf151e8fc659007a5e9180810c1b8eb852aa98","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"32f085f6_0b825fe8","updated":"2024-10-22 15:15:45.000000000","message":"LGTM, zuul failures are looks unrelated.","commit_id":"02b7ec4aac21ac05850f0e56133890824217146a"}],"nova/compute/api.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"14cf307de71ed7c7c9a507aa3dd21ba74784b53b","unresolved":true,"context_lines":[{"line_number":3976,"context_line":"        # when we revert resize the instance."},{"line_number":3977,"context_line":"        old_image_min_disk \u003d instance.old_flavor.get(\"root_gb\")"},{"line_number":3978,"context_line":"        instance.system_metadata.update("},{"line_number":3979,"context_line":"             {\"image_min_disk\": old_image_min_disk})"},{"line_number":3980,"context_line":"        instance.save(expected_task_state\u003d[None])"},{"line_number":3981,"context_line":""},{"line_number":3982,"context_line":"        migration.status \u003d \u0027reverting\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"ac8e0694_44f14858","line":3979,"updated":"2024-10-29 23:49:00.000000000","message":"I don\u0027t think updating the instance system_metadata is the right approach for fixing the issue. The `image_` prefixed properties come from the original image used to create the instance and are generally meant to be immutable.\n\nI think instead the snapshot image metadata should be initialized with the correct value for `min_disk`.","commit_id":"02b7ec4aac21ac05850f0e56133890824217146a"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"444a04af52ba0b967b99ae6fa6f0c967c2ecf03b","unresolved":true,"context_lines":[{"line_number":3976,"context_line":"        # when we revert resize the instance."},{"line_number":3977,"context_line":"        old_image_min_disk \u003d instance.old_flavor.get(\"root_gb\")"},{"line_number":3978,"context_line":"        instance.system_metadata.update("},{"line_number":3979,"context_line":"             {\"image_min_disk\": old_image_min_disk})"},{"line_number":3980,"context_line":"        instance.save(expected_task_state\u003d[None])"},{"line_number":3981,"context_line":""},{"line_number":3982,"context_line":"        migration.status \u003d \u0027reverting\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"5641619c_bb1f5a60","line":3979,"in_reply_to":"ac8e0694_44f14858","updated":"2024-12-24 03:47:50.000000000","message":"i agree we should not update the instance_system_metadata on the instance that is being snapshotted","commit_id":"02b7ec4aac21ac05850f0e56133890824217146a"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"444a04af52ba0b967b99ae6fa6f0c967c2ecf03b","unresolved":true,"context_lines":[{"line_number":4324,"context_line":"        # resize the instance. So that, min disk of snapshot to create"},{"line_number":4325,"context_line":"        # instance from will be the root_gb of new flavor."},{"line_number":4326,"context_line":"        instance.system_metadata.update("},{"line_number":4327,"context_line":"                {\"image_min_disk\": new_flavor.get(\"root_gb\")})"},{"line_number":4328,"context_line":"        instance.save(expected_task_state\u003d[None])"},{"line_number":4329,"context_line":""},{"line_number":4330,"context_line":"        if not flavor_id:"}],"source_content_type":"text/x-python","patch_set":6,"id":"4aac4bbb_37622861","line":4327,"updated":"2024-12-24 03:47:50.000000000","message":"we cant in genreally only look at the current flavor.\nthat will work in some cases but not for BFV flavors with that set to 0\n\nif we are to update teh image property on the snapshot we need to use the logical image size of the backing storage rather then the currently allcoated size.\n\nfor qcow images and thin provisioned cinder volumes the allocated paces can be smaller the the logical size of the disk so we need to use the logcial size.\n\n\nThe BFV case is tricky as the fake glance image we create is kind of a hack so im now sure what we want the behavior to be in that case.\nwe also dont know if the bfv volume has been resized via cinder.\n\n\nperhaps we should set the min_disk to the max(current_value, flavor_value) for non bfv instnaces and unset the field entirely if its boot form voluem?","commit_id":"02b7ec4aac21ac05850f0e56133890824217146a"}],"nova/compute/utils.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"14cf307de71ed7c7c9a507aa3dd21ba74784b53b","unresolved":true,"context_lines":[{"line_number":1302,"context_line":""},{"line_number":1303,"context_line":"    # The properties in extra_properties have precedence"},{"line_number":1304,"context_line":"    properties.update(extra_properties)"},{"line_number":1305,"context_line":""},{"line_number":1306,"context_line":"    return image_meta"},{"line_number":1307,"context_line":""},{"line_number":1308,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"86dd4127_9cd6cd53","line":1305,"updated":"2024-10-29 23:49:00.000000000","message":"I think this is where a fix should go. We can initialize the `min_disk` property here using what\u0027s in the flavor. I think we should probably set `min_ram` as well?","commit_id":"02b7ec4aac21ac05850f0e56133890824217146a"},{"author":{"_account_id":35983,"name":"Aleksandr Chudinov","email":"silentirk@me.com","username":"silentirk"},"change_message_id":"ff3954c2da94ad1c7d5ba920e67d33eb9d63d6cb","unresolved":true,"context_lines":[{"line_number":1302,"context_line":""},{"line_number":1303,"context_line":"    # The properties in extra_properties have precedence"},{"line_number":1304,"context_line":"    properties.update(extra_properties)"},{"line_number":1305,"context_line":""},{"line_number":1306,"context_line":"    return image_meta"},{"line_number":1307,"context_line":""},{"line_number":1308,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"f8f289c0_f50ed25e","line":1305,"in_reply_to":"86dd4127_9cd6cd53","updated":"2024-12-23 10:05:39.000000000","message":"I think we should not set min_ram as ram consumption/requirement is more dynamic thing and not directly related to the possibility to spawn instances as opposed to the disk size, which is directly required by snapshot itself.","commit_id":"02b7ec4aac21ac05850f0e56133890824217146a"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"444a04af52ba0b967b99ae6fa6f0c967c2ecf03b","unresolved":true,"context_lines":[{"line_number":1302,"context_line":""},{"line_number":1303,"context_line":"    # The properties in extra_properties have precedence"},{"line_number":1304,"context_line":"    properties.update(extra_properties)"},{"line_number":1305,"context_line":""},{"line_number":1306,"context_line":"    return image_meta"},{"line_number":1307,"context_line":""},{"line_number":1308,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"f3037e52_1cc30c68","line":1305,"in_reply_to":"f8f289c0_f50ed25e","updated":"2024-12-24 03:47:50.000000000","message":"min ram is a property of the orgianl image but we should not update it on resize in my opipion.\n\nthis entire bug is borderline, i can see the intent in preventing a boot failure when trying to create a new instance from the image, but i think we should avoid changing other fields.","commit_id":"02b7ec4aac21ac05850f0e56133890824217146a"}],"nova/tests/unit/compute/test_api.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"14cf307de71ed7c7c9a507aa3dd21ba74784b53b","unresolved":true,"context_lines":[{"line_number":1887,"context_line":"        else:"},{"line_number":1888,"context_line":"            fake_inst.old_flavor \u003d self._create_flavor("},{"line_number":1889,"context_line":"                id\u003d200, flavorid\u003d\u0027new-flavor-id\u0027, name\u003d\u0027new_flavor\u0027,"},{"line_number":1890,"context_line":"                disabled\u003dFalse, extra_specs\u003d{\u0027hw:numa_nodes\u0027: \u00271\u0027}, root_gb\u003d1)"},{"line_number":1891,"context_line":""},{"line_number":1892,"context_line":"        mock_elevated.return_value \u003d self.context"},{"line_number":1893,"context_line":"        mock_get_migration.return_value \u003d fake_mig"}],"source_content_type":"text/x-python","patch_set":6,"id":"4b09fe1c_b53a79ff","line":1890,"updated":"2024-10-29 23:49:00.000000000","message":"Instead of these unit test changes, I think better test would be a functional test which creates an instance, takes a snapshot, and then verifies the image properties from the snapshot image. I took a quick look at the `GlanceFixture` and I think it should be able to handle a scenario like this.\n\nI don\u0027t know if you are already familiar but what we usually do is write a separate patch for the functional test and assert the broken behavior in it along with a FIXME code comment. Then, we stack the patch with the fix on top and assert the fixed behavior and remove the FIXME comment.\n\nLet me know if you have questions about this or if you will not be able to write such a test. In the latter case I could contribute the test patch if needed.","commit_id":"02b7ec4aac21ac05850f0e56133890824217146a"},{"author":{"_account_id":35983,"name":"Aleksandr Chudinov","email":"silentirk@me.com","username":"silentirk"},"change_message_id":"ff3954c2da94ad1c7d5ba920e67d33eb9d63d6cb","unresolved":true,"context_lines":[{"line_number":1887,"context_line":"        else:"},{"line_number":1888,"context_line":"            fake_inst.old_flavor \u003d self._create_flavor("},{"line_number":1889,"context_line":"                id\u003d200, flavorid\u003d\u0027new-flavor-id\u0027, name\u003d\u0027new_flavor\u0027,"},{"line_number":1890,"context_line":"                disabled\u003dFalse, extra_specs\u003d{\u0027hw:numa_nodes\u0027: \u00271\u0027}, root_gb\u003d1)"},{"line_number":1891,"context_line":""},{"line_number":1892,"context_line":"        mock_elevated.return_value \u003d self.context"},{"line_number":1893,"context_line":"        mock_get_migration.return_value \u003d fake_mig"}],"source_content_type":"text/x-python","patch_set":6,"id":"0c42209c_7269a000","line":1890,"in_reply_to":"4b09fe1c_b53a79ff","updated":"2024-12-23 10:05:39.000000000","message":"Big thanks for help!\nI added a change with the test:\nhttps://review.opendev.org/c/openstack/nova/+/938173\n\nCould you please look at it, when you have time?","commit_id":"02b7ec4aac21ac05850f0e56133890824217146a"}]}