)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"0e005266c063c0203628fcaba70865bdf02d6cf3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"d1f017f4_154dbe51","updated":"2024-08-15 14:51:11.000000000","message":"I need to still fix the unit test and look into the free_instance_claims comment","commit_id":"2d4508fea65e64cd1b985c79ee5e616487ec7fc7"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"0a2e1cc47a630512f48404ed8dfb90128ea261d6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"3a77aa0c_8d809497","updated":"2024-08-16 10:14:06.000000000","message":"Now I expect the unit and functional test to pass. Then I will look at the remaining comments.","commit_id":"a631d782bd8f164bfc175cb8d7d63807abc87c7a"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c9174cb9d50ee1bf233e907fce442f2a03f21137","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"28628c40_bfae59f1","updated":"2024-08-16 12:29:57.000000000","message":"nova-next failure can be relevant:\n```\n      File \"/opt/stack/tempest/tempest/api/compute/admin/test_servers_on_multinodes.py\", line 178, in test_unshelve_to_specific_host\n    self.assertEqual(otherhost, self.get_host_for_server(server[\u0027id\u0027]))\n\n      File \"/opt/stack/tempest/.tox/tempest/lib/python3.10/site-packages/testtools/testcase.py\", line 419, in assertEqual\n    self.assertThat(observed, matcher, message)\n\n      File \"/opt/stack/tempest/.tox/tempest/lib/python3.10/site-packages/testtools/testcase.py\", line 509, in assertThat\n    raise mismatch_error\n\n    testtools.matchers._impl.MismatchError: \u0027np0038203191\u0027 !\u003d None\n```","commit_id":"a631d782bd8f164bfc175cb8d7d63807abc87c7a"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"137898d389df5e80e29a8392b52bd78630cee770","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"70675ff9_b0a4a15c","in_reply_to":"28628c40_bfae59f1","updated":"2024-08-16 14:11:05.000000000","message":"The instance spawned on the host successfully:\nAug 16 10:52:31.960644 np0038203191 nova-compute[44776]: INFO nova.virt.libvirt.driver [-] [instance: 4bcad078-d11d-430b-8e50-00a1ee5f1fa9] Instance spawned successfully.\n33743\t\n\nbut the nova API returned no host for the instance:\n\n\"OS-EXT-SRV-ATTR:host\": null, \"OS-EXT-SRV-ATTR:instance_name\": \"instance-00000054\", \"OS-EXT-SRV-ATTR:hypervisor_hostname\": null\n\nThis commit changes the way how we abort a claim but not the way how we create the claim and this test case did not aborted any claim so I think this is not related. My current assumption that the instance state might reach ACTIVE before the instane.host is changed (but that sounds strange), and therefore is tempest asks the state at the right time then it can see the host being still null. \n\nI will need to respin this patch anyhow so I will test if this was a one off or a stable failure.","commit_id":"a631d782bd8f164bfc175cb8d7d63807abc87c7a"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"cc3f008f6e7656a6d19ec7d934af75c4cbd5fceb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"c48fdc65_7b450ee7","updated":"2024-08-16 18:52:22.000000000","message":"Sorry to say I don\u0027t understand enough about the wider code here to feel qualified to +2 this.","commit_id":"f8b98390dc99f6cb0101c88223eb840e0d1c7124"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"858bcbf124cd568c2a94c72d7893983de980f026","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"601a8793_aa09ae47","updated":"2024-08-19 15:15:20.000000000","message":"This makes sense to me","commit_id":"f8b98390dc99f6cb0101c88223eb840e0d1c7124"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"0dea0b123d3d8a8328ffee829f61dc92d12eb00c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"2bccee4c_64a003ac","updated":"2024-08-16 17:15:38.000000000","message":"thanks gibi this looks like a better solution then the instnace.refesh to me so i think it woudl be reasonable to proceed with this approch.","commit_id":"f8b98390dc99f6cb0101c88223eb840e0d1c7124"}],"doc/notification_samples/instance-create-error.json":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a93dd6d254189312655ce5e5ffdea0b0102ff6b0","unresolved":true,"context_lines":[{"line_number":20,"context_line":"            \"power_state\": \"pending\","},{"line_number":21,"context_line":"            \"state\": \"building\","},{"line_number":22,"context_line":"            \"host\": null,"},{"line_number":23,"context_line":"            \"node\": null"},{"line_number":24,"context_line":"        }"},{"line_number":25,"context_line":"    },"},{"line_number":26,"context_line":"    \"priority\":\"ERROR\","}],"source_content_type":"application/json","patch_set":3,"id":"5b95ca02_c12bb066","line":23,"updated":"2024-08-16 10:51:09.000000000","message":"ah yes this is because we are correctly resettting this now +1","commit_id":"4424c00fe11f815fb9937b53c1bc14d1d0657456"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"b07172658eda3724e98692623e84cb52050fc6f0","unresolved":false,"context_lines":[{"line_number":20,"context_line":"            \"power_state\": \"pending\","},{"line_number":21,"context_line":"            \"state\": \"building\","},{"line_number":22,"context_line":"            \"host\": null,"},{"line_number":23,"context_line":"            \"node\": null"},{"line_number":24,"context_line":"        }"},{"line_number":25,"context_line":"    },"},{"line_number":26,"context_line":"    \"priority\":\"ERROR\","}],"source_content_type":"application/json","patch_set":3,"id":"0efe13d6_f1a78b2b","line":23,"in_reply_to":"5b95ca02_c12bb066","updated":"2024-08-16 14:12:15.000000000","message":"Acknowledged","commit_id":"4424c00fe11f815fb9937b53c1bc14d1d0657456"}],"nova/compute/claims.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"6d412b3be170f03b016ccc68455068bfd3e380ad","unresolved":true,"context_lines":[{"line_number":83,"context_line":"        been aborted."},{"line_number":84,"context_line":"        \"\"\""},{"line_number":85,"context_line":"        LOG.debug(\"Aborting claim: %s\", self, instance\u003dself.instance)"},{"line_number":86,"context_line":"        self.tracker.abort_instance_claim(self.context, self.instance_ref,"},{"line_number":87,"context_line":"                                          self.nodename)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def _claim_test(self, compute_node, limits\u003dNone):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5d65d553_e694a096","line":86,"updated":"2024-08-15 11:23:54.000000000","message":"This is a big change so it might tease out other bugs in the cleanup logic.","commit_id":"0c3001a4fd1e981e3785e2c40cb7cd9969d6f789"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ad2b94f2b0c972225c5e84cb01e214416bc7a622","unresolved":true,"context_lines":[{"line_number":83,"context_line":"        been aborted."},{"line_number":84,"context_line":"        \"\"\""},{"line_number":85,"context_line":"        LOG.debug(\"Aborting claim: %s\", self, instance\u003dself.instance)"},{"line_number":86,"context_line":"        self.tracker.abort_instance_claim(self.context, self.instance_ref,"},{"line_number":87,"context_line":"                                          self.nodename)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def _claim_test(self, compute_node, limits\u003dNone):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5c17979b_2669cc38","line":86,"in_reply_to":"3067885b_eefb6db5","updated":"2024-08-15 13:41:10.000000000","message":"ya i was going to say we really should be resetting them to none if the claim failes so i agree.","commit_id":"0c3001a4fd1e981e3785e2c40cb7cd9969d6f789"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"2b63615fe64582a867f0ec46d0aaf9c168b46af9","unresolved":false,"context_lines":[{"line_number":83,"context_line":"        been aborted."},{"line_number":84,"context_line":"        \"\"\""},{"line_number":85,"context_line":"        LOG.debug(\"Aborting claim: %s\", self, instance\u003dself.instance)"},{"line_number":86,"context_line":"        self.tracker.abort_instance_claim(self.context, self.instance_ref,"},{"line_number":87,"context_line":"                                          self.nodename)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def _claim_test(self, compute_node, limits\u003dNone):"}],"source_content_type":"text/x-python","patch_set":1,"id":"fd03ace8_cff3395e","line":86,"in_reply_to":"5c17979b_2669cc38","updated":"2024-08-16 08:48:48.000000000","message":"fixed the notification sample to reflect the new behavior.","commit_id":"0c3001a4fd1e981e3785e2c40cb7cd9969d6f789"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"7a000123e905ca631ba8f11aba40f637b67603e5","unresolved":true,"context_lines":[{"line_number":83,"context_line":"        been aborted."},{"line_number":84,"context_line":"        \"\"\""},{"line_number":85,"context_line":"        LOG.debug(\"Aborting claim: %s\", self, instance\u003dself.instance)"},{"line_number":86,"context_line":"        self.tracker.abort_instance_claim(self.context, self.instance_ref,"},{"line_number":87,"context_line":"                                          self.nodename)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def _claim_test(self, compute_node, limits\u003dNone):"}],"source_content_type":"text/x-python","patch_set":1,"id":"3067885b_eefb6db5","line":86,"in_reply_to":"5d65d553_e694a096","updated":"2024-08-15 11:51:35.000000000","message":"There is one functional test failure in nova.tests.functional.notification_sample_tests.test_instance.TestInstanceNotificationSample.test_create_server_error\nIt is due to the fact that abort_instance_claim reset the instance.host and .node to None in the DB but before this commit it is not reflected on the instance object used by the compute manager so the notification sent by the compute manager still has the .host and .node set to the compute. This change of behavior is also a bugfix as that the instance notification sent at reschedule now better aligned with the data in the DB.","commit_id":"0c3001a4fd1e981e3785e2c40cb7cd9969d6f789"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e2879548f0fca2c2cb4ee87621983083f321e152","unresolved":true,"context_lines":[{"line_number":78,"context_line":"    def numa_topology(self):"},{"line_number":79,"context_line":"        return self.instance.numa_topology"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def abort(self):"},{"line_number":82,"context_line":"        \"\"\"Compute operation requiring claimed resources has failed or"},{"line_number":83,"context_line":"        been aborted."},{"line_number":84,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"fc3d77c1_494d7be7","line":81,"updated":"2024-08-16 22:55:33.000000000","message":"@melwittt@gmail.com abort is only called if the claim was successfully but we then later fail to boot the vm.\n\nin the case of the bug, at least downstream, nova was correctly able ot claim the pci device in our db because it was free but then libvirt was not able to assign the device.\n\n\nthe claim logic in this file just ensure we have enough pci device to fit the request and computes the instance numa toplogy and ensure it will fit too.\n\nthis is all done in memory at this poitn and the selection of which specific pci device is allocated is done later.\n\nso the the instance claim object was created in the resouce tracker here\n\nhttps://github.com/openstack/nova/blob/a7c82399b237aff44810ac76b0c4d10416c3bdf9/nova/compute/resource_tracker.py#L190-L191\n\nand then  the pci devices were claimed in the db here\nhttps://github.com/openstack/nova/blob/a7c82399b237aff44810ac76b0c4d10416c3bdf9/nova/compute/resource_tracker.py#L201-L206\n\n\n_update_usage_from_instance is invoked here\nhttps://github.com/openstack/nova/blob/a7c82399b237aff44810ac76b0c4d10416c3bdf9/nova/compute/resource_tracker.py#L211\n\nwhich updates the pci device in the pci tracker here\n\nhttps://github.com/openstack/nova/blob/a7c82399b237aff44810ac76b0c4d10416c3bdf9/nova/compute/resource_tracker.py#L1634-L1636\nand moves moves the pci devies to allocated.\n\nhttps://github.com/openstack/nova/blob/a7c82399b237aff44810ac76b0c4d10416c3bdf9/nova/pci/manager.py#L457\n\n\nthe abort_instance_claim gibi is adding undoes that work \n\nby calling _update_usage_from_instance again with  is_removed\u003dtrue\nhttps://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L580-L581\n\nprviously this was being called on the cached copy of the instance\nbut the actual instance object was used for the allocation of the pci device.\n\nhttps://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L204-L206\n\nthat is why i bleive this fixes it as we are now calling abort on the same instnace objected via instance_ref\n\nthis is all supper squirly and we proably should not be doing the pci_tracker claims sepreatly form the claim object in the resouce tracker liek we are today.\n\ni personally hate the claim object and want them to not exist so i have never been motivated to change this as i dont like that claims are not atomicly done in the db and only exist in memory.\n\nwhich is why i did not extend them when adding sriov live migration.","commit_id":"f8b98390dc99f6cb0101c88223eb840e0d1c7124"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"06d661364259bbbe45a0ee2e6f161cbaf67b614a","unresolved":true,"context_lines":[{"line_number":78,"context_line":"    def numa_topology(self):"},{"line_number":79,"context_line":"        return self.instance.numa_topology"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def abort(self):"},{"line_number":82,"context_line":"        \"\"\"Compute operation requiring claimed resources has failed or"},{"line_number":83,"context_line":"        been aborted."},{"line_number":84,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"acd35280_07a77a14","line":81,"in_reply_to":"97ca2913_591e55f9","updated":"2024-08-26 13:33:20.000000000","message":"so I think it would be worth creating a follow-up patch to try and drop the clone entirely and see what happens.\n\nI don\u0027t really think it is serving a clear purpose anymore and if it is required it would be good to document it so it is probably worth trying to move everything directly to the instance object and see if there is any test fallout.\n\nif nothing else that will give us another data point to consider.","commit_id":"f8b98390dc99f6cb0101c88223eb840e0d1c7124"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"24d6dd504a533c3f5ebaf7fe369ca3811c913cf4","unresolved":true,"context_lines":[{"line_number":78,"context_line":"    def numa_topology(self):"},{"line_number":79,"context_line":"        return self.instance.numa_topology"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def abort(self):"},{"line_number":82,"context_line":"        \"\"\"Compute operation requiring claimed resources has failed or"},{"line_number":83,"context_line":"        been aborted."},{"line_number":84,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"97ca2913_591e55f9","line":81,"in_reply_to":"c27dccc7_3934ed8e","updated":"2024-08-26 07:34:11.000000000","message":"@melwittt@gmail.com I cannot give a full answer on the unwanted side effect question other than all our existing in tree testing passes on the change. \n\nThere is a bit more context in the comments of the original fix proposal in https://review.opendev.org/c/openstack/nova/+/710848 detailing how I reached the root cause (the clone). \n\n\nThe symmetry of using the clone is already broken by the instance_claim method on the resource tracker as not all changes operates on the Claim object there with a clone but some, like PCI, operates directly on the instance. However during the abort all operation including PCI operates on the clone. So this patch not breaks symmetry instead it makes sure that how we operate in the instance_claim (even if it feels overly complicated) symmetric with how we operate in the abort case. \n\nStill it is a valid question if we want that all operation work on the clone or all operation work directly on the object. However I did not want to blow up the scope of the bug fix with such changes. (To be honest I afraid of unwanted side effects there too)","commit_id":"f8b98390dc99f6cb0101c88223eb840e0d1c7124"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"522547134343acb1150c9aee8656010a86352533","unresolved":true,"context_lines":[{"line_number":78,"context_line":"    def numa_topology(self):"},{"line_number":79,"context_line":"        return self.instance.numa_topology"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def abort(self):"},{"line_number":82,"context_line":"        \"\"\"Compute operation requiring claimed resources has failed or"},{"line_number":83,"context_line":"        been aborted."},{"line_number":84,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"c27dccc7_3934ed8e","line":81,"in_reply_to":"cc235438_0e74f8cc","updated":"2024-08-19 20:08:16.000000000","message":"i suspect that we could safely remove   self.instance \u003d instance.obj_clone()\n\nandjust use self.instance_ref\n\nhowever some of the methods in hardware.py do modify there in puts and i didn to survay the ones we are callling to make sure its safe to always use the opject instead of the clone.","commit_id":"f8b98390dc99f6cb0101c88223eb840e0d1c7124"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"9ea38d67280cba3c9ad812a0b4c8f588ab953b4a","unresolved":true,"context_lines":[{"line_number":78,"context_line":"    def numa_topology(self):"},{"line_number":79,"context_line":"        return self.instance.numa_topology"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def abort(self):"},{"line_number":82,"context_line":"        \"\"\"Compute operation requiring claimed resources has failed or"},{"line_number":83,"context_line":"        been aborted."},{"line_number":84,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":5,"id":"cc235438_0e74f8cc","line":81,"in_reply_to":"fc3d77c1_494d7be7","updated":"2024-08-19 17:31:51.000000000","message":"Thanks for the explanation here and in your other reply. I did understand why instance_ref works but I did not understand whether there is symmetry for other types of claims that are not PCI. Do other claims mutate the original object? If they do, then instance_ref in abort makes sense for all cases. If other claims do not mutate the original object, then I wondered if there could be any consequence to the asymmetry? And if there isn\u0027t, it would have been nice to have a code comment explaining why the asymmetry is OK, for future readers of this code.","commit_id":"f8b98390dc99f6cb0101c88223eb840e0d1c7124"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"cc3f008f6e7656a6d19ec7d934af75c4cbd5fceb","unresolved":true,"context_lines":[{"line_number":84,"context_line":"        \"\"\""},{"line_number":85,"context_line":"        LOG.debug(\"Aborting claim: %s\", self, instance\u003dself.instance)"},{"line_number":86,"context_line":"        self.tracker.abort_instance_claim(self.context, self.instance_ref,"},{"line_number":87,"context_line":"                                          self.nodename)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def _claim_test(self, compute_node, limits\u003dNone):"},{"line_number":90,"context_line":"        \"\"\"Test if this claim can be satisfied given available resources and"}],"source_content_type":"text/x-python","patch_set":5,"id":"c14f8c1c_0cfd68f9","line":87,"updated":"2024-08-16 18:52:22.000000000","message":"This looks simple enough and I understand why it addresses the problem. But TBH I don\u0027t understand anything else like the reason there is an instance clone in the first place and how to know this is safe/doesn\u0027t have undesired side effects? And what about claim of resources other than PCI, do those use the original instance or the clone? If it\u0027s the former, then it would seem clearer why this is the right thing to do.\n\nI read through the bug comments and the old patch reviews https://review.opendev.org/c/openstack/nova/+/710847 and https://review.opendev.org/c/openstack/nova/+/710848 and did not catch any explanation why/how this is a safe change.\n\nIf we know more info about what the clone is, what it\u0027s for, and which instance (clone vs original) other types (non PCI) of claims mutate then I think `__init__()` and `abort()` would benefit a lot from a code comment or two.","commit_id":"f8b98390dc99f6cb0101c88223eb840e0d1c7124"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e2879548f0fca2c2cb4ee87621983083f321e152","unresolved":true,"context_lines":[{"line_number":84,"context_line":"        \"\"\""},{"line_number":85,"context_line":"        LOG.debug(\"Aborting claim: %s\", self, instance\u003dself.instance)"},{"line_number":86,"context_line":"        self.tracker.abort_instance_claim(self.context, self.instance_ref,"},{"line_number":87,"context_line":"                                          self.nodename)"},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"    def _claim_test(self, compute_node, limits\u003dNone):"},{"line_number":90,"context_line":"        \"\"\"Test if this claim can be satisfied given available resources and"}],"source_content_type":"text/x-python","patch_set":5,"id":"eac4a7fc_f442a90a","line":87,"in_reply_to":"c14f8c1c_0cfd68f9","updated":"2024-08-16 22:55:33.000000000","message":"this is one of the few parts of the numa code that actully predates me workign on nova.  i was still trying to get devstack and neutron to supprot ovdk at the time and learning what openstack was.\n\nin fact the presence of a copy of the instance in the claim exists form the very early day sof instnace claims.\n\n\nmy understainding is that orginally we made a copy of the instance by confverting it to an dict  and later we change to using objects whihc intoduced a clone\n\nhttps://github.com/openstack/nova/commit/45f1c598b71945f0ea4bc26c21481762a22db9a7\n\ni belive the inte was that durign the claim process we would not modify the instance in the db if we endedup mofiying the object in the claim and we were effectivly usign it to cache the instance info in memory to avoid doing a db lookup any time we wanted to lookup data related to the instance form the claim.\n\nso i belive the intent of it being a clone of snapshot of the isntance when the claim was created was to isolate the db code from the in memory object so that if a cliam failed no changes were propagated into the object.","commit_id":"f8b98390dc99f6cb0101c88223eb840e0d1c7124"}],"nova/pci/manager.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ad2b94f2b0c972225c5e84cb01e214416bc7a622","unresolved":true,"context_lines":[{"line_number":407,"context_line":"        for dev in self.pci_devs:"},{"line_number":408,"context_line":"            if (dev.status \u003d\u003d fields.PciDeviceStatus.ALLOCATED and"},{"line_number":409,"context_line":"                    dev.instance_uuid \u003d\u003d instance[\u0027uuid\u0027]):"},{"line_number":410,"context_line":"                self._free_device(dev, instance)"},{"line_number":411,"context_line":""},{"line_number":412,"context_line":"    def free_instance_claims("},{"line_number":413,"context_line":"        self, context: ctx.RequestContext, instance: \u0027objects.Instance\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"935509a7_6c4ae523","line":410,"updated":"2024-08-15 13:41:10.000000000","message":"well this is subtle","commit_id":"0c3001a4fd1e981e3785e2c40cb7cd9969d6f789"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"137898d389df5e80e29a8392b52bd78630cee770","unresolved":false,"context_lines":[{"line_number":407,"context_line":"        for dev in self.pci_devs:"},{"line_number":408,"context_line":"            if (dev.status \u003d\u003d fields.PciDeviceStatus.ALLOCATED and"},{"line_number":409,"context_line":"                    dev.instance_uuid \u003d\u003d instance[\u0027uuid\u0027]):"},{"line_number":410,"context_line":"                self._free_device(dev, instance)"},{"line_number":411,"context_line":""},{"line_number":412,"context_line":"    def free_instance_claims("},{"line_number":413,"context_line":"        self, context: ctx.RequestContext, instance: \u0027objects.Instance\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"f2815bd7_656a2b0c","line":410,"in_reply_to":"935509a7_6c4ae523","updated":"2024-08-16 14:11:05.000000000","message":"Acknowledged","commit_id":"0c3001a4fd1e981e3785e2c40cb7cd9969d6f789"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ad2b94f2b0c972225c5e84cb01e214416bc7a622","unresolved":true,"context_lines":[{"line_number":423,"context_line":"        for dev in self.pci_devs:"},{"line_number":424,"context_line":"            if (dev.status \u003d\u003d fields.PciDeviceStatus.CLAIMED and"},{"line_number":425,"context_line":"                    dev.instance_uuid \u003d\u003d instance[\u0027uuid\u0027]):"},{"line_number":426,"context_line":"                self._free_device(dev)"},{"line_number":427,"context_line":""},{"line_number":428,"context_line":"    def free_instance("},{"line_number":429,"context_line":"        self, context: ctx.RequestContext, instance: \u0027objects.Instance\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"7715338f_c5dfb8b0","line":426,"updated":"2024-08-15 13:41:10.000000000","message":"i need to think about this but im inclide to think we should be passing instnace here too?","commit_id":"0c3001a4fd1e981e3785e2c40cb7cd9969d6f789"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"137898d389df5e80e29a8392b52bd78630cee770","unresolved":false,"context_lines":[{"line_number":423,"context_line":"        for dev in self.pci_devs:"},{"line_number":424,"context_line":"            if (dev.status \u003d\u003d fields.PciDeviceStatus.CLAIMED and"},{"line_number":425,"context_line":"                    dev.instance_uuid \u003d\u003d instance[\u0027uuid\u0027]):"},{"line_number":426,"context_line":"                self._free_device(dev)"},{"line_number":427,"context_line":""},{"line_number":428,"context_line":"    def free_instance("},{"line_number":429,"context_line":"        self, context: ctx.RequestContext, instance: \u0027objects.Instance\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"53c3680f_65476620","line":426,"in_reply_to":"7715338f_c5dfb8b0","updated":"2024-08-16 14:11:05.000000000","message":"Done.","commit_id":"0c3001a4fd1e981e3785e2c40cb7cd9969d6f789"}],"nova/tests/unit/compute/test_shelve.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"2b63615fe64582a867f0ec46d0aaf9c168b46af9","unresolved":true,"context_lines":[{"line_number":600,"context_line":"                # This is after we\u0027ve failed."},{"line_number":601,"context_line":"                self.assertIsNone(instance.host)"},{"line_number":602,"context_line":"                self.assertIsNone(instance.node)"},{"line_number":603,"context_line":"                self.assertIsNone(instance.task_state)"},{"line_number":604,"context_line":"                tracking[\u0027last_state\u0027] \u003d instance.task_state"},{"line_number":605,"context_line":"            else:"},{"line_number":606,"context_line":"                self.fail(\u0027Unexpected save!\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"d9104d1a_cbaf79ad","side":"PARENT","line":603,"updated":"2024-08-16 08:48:48.000000000","message":"This test change is due to the test setup. The test mocks the instance.save to check for state transition. This instance object is the original one from the compute manager. So before this fix it did not see the save() call that happened on the cloned instance during the claim abort. Now it see it and fails as that save does not reset the task_state, the task state is reset in the next save in the reverts_task_state decorator when the exception bubbles up out of the unshelve method.\n\nSo instead of asserting the state transition the test is modified to assert the end state.","commit_id":"a2d77845ab247f1b09e2ae4f32f9421c3f50b98d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a93dd6d254189312655ce5e5ffdea0b0102ff6b0","unresolved":true,"context_lines":[{"line_number":600,"context_line":"                # This is after we\u0027ve failed."},{"line_number":601,"context_line":"                self.assertIsNone(instance.host)"},{"line_number":602,"context_line":"                self.assertIsNone(instance.node)"},{"line_number":603,"context_line":"                self.assertIsNone(instance.task_state)"},{"line_number":604,"context_line":"                tracking[\u0027last_state\u0027] \u003d instance.task_state"},{"line_number":605,"context_line":"            else:"},{"line_number":606,"context_line":"                self.fail(\u0027Unexpected save!\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"7e7d4a13_0a62f647","side":"PARENT","line":603,"in_reply_to":"d9104d1a_cbaf79ad","updated":"2024-08-16 10:51:09.000000000","message":"that makes sesnes.","commit_id":"a2d77845ab247f1b09e2ae4f32f9421c3f50b98d"}],"nova/tests/unit/pci/test_manager.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"0a2e1cc47a630512f48404ed8dfb90128ea261d6","unresolved":true,"context_lines":[{"line_number":530,"context_line":"        self.tracker.free_instance("},{"line_number":531,"context_line":"            mock.sentinel.context,"},{"line_number":532,"context_line":"            {\u0027uuid\u0027: uuidsentinel.instance1,"},{"line_number":533,"context_line":"             \u0027pci_devices\u0027: [objects.PciDevice(id\u003dpf[\u0027id\u0027])]})"},{"line_number":534,"context_line":""},{"line_number":535,"context_line":"        pf_dev \u003d self._get_device_by_address(pf[\u0027address\u0027])"},{"line_number":536,"context_line":"        self.assertEqual(\u0027available\u0027, pf_dev.status)"}],"source_content_type":"text/x-python","patch_set":4,"id":"8e8b0f90_4784c15b","line":533,"updated":"2024-08-16 10:14:06.000000000","message":"this and the similar change below is needed as free is now manipulating the instance.pci_devices list as well.","commit_id":"a631d782bd8f164bfc175cb8d7d63807abc87c7a"}]}