)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"007c25102279b010a40b3d850c66a32e7af5b7b9","unresolved":false,"context_lines":[{"line_number":13,"context_line":"with read_deleted\u003dyes."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: Ide6cc5bb1fce2c9aea9fa3efdf940e8308cd9ed0"},{"line_number":16,"context_line":"Closes-Bug: #1745977"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3fa0c359_18b836c6","line":16,"updated":"2018-02-19 21:22:54.000000000","message":"Looking at this bug, it was introduced with this change in pike, which was also backported to ocata:\n\nhttps://review.openstack.org/#/q/Ib5f6b03189b7fc5cd0b226ea2dca74865fbef12a\n\nWhere we used to only pull non-deleted migration/instance records from the DB during cleanup of an evacuated host once it comes back online.\n\nAnd it looks like the failure is on accessing instance.system_metadata:\n\n2018-01-29 10:28:48.664 9364 ERROR oslo_service.service File \"/usr/lib/python2.7/site-packages/nova/virt/libvirt/driver.py\", line 1032, in cleanup\n2018-01-29 10:28:48.664 9364 ERROR oslo_service.service attempts \u003d int(instance.system_metadata.get(\u0027clean_attempts\u0027,\n\nIf someone archived their db after evacuating the instances and before bringing the source compute back up, then we shouldn\u0027t even get those instances here:\n\nhttps://github.com/openstack/nova/blob/stable/pike/nova/compute/manager.py#L651\n\nBecause the DB wouldn\u0027t return them.\n\nSo then, yeah, our problem is the lazy-load. The generic fix handles that case and hopefully others, but this could still be a game of whack-a-mole.\n\nWe also have a follow up change for this bug which is that _get_instances_on_driver should be passing at least \u0027system_metadata\u0027 for expected_attrs when getting the InstanceList.","commit_id":"6ba8a35825a7ec839b2d0aab7559351d573130ab"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"3c5276e8a765f871427ca13b5ec191e49b69e7a8","unresolved":false,"context_lines":[{"line_number":13,"context_line":"with read_deleted\u003dyes."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: Ide6cc5bb1fce2c9aea9fa3efdf940e8308cd9ed0"},{"line_number":16,"context_line":"Closes-Bug: #1745977"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3fa0c359_5821defa","line":16,"in_reply_to":"3fa0c359_18b836c6","updated":"2018-02-19 21:30:24.000000000","message":"Yep, that looks like the culprit to me. Without that regression, I still think this makes sense for us to do. If we\u0027re ever in a delete cleanup routine then we don\u0027t want to nuke the cleanup process because we hit something like this.\n\nEarly on we talked about doing this, but decided that we should use those situations to find and squash lazy-loads. However, during a delete cleanup especially we may be coming from weird locations (like _get_instances_on_driver() as opposed to a request from the API), which is where we want to be extra sure we don\u0027t crater on something silly.\n\nThe whole point of our soft deleting of database records is so we can continue to have these relationships for visibly-deleted records, so failing on something like this is kinda dumb.\n\nWe could also just catch the NotFound and try to fake out some empty data instead of actually loading it, but that\u0027s more likely to make the cleanup process fail (or fail to find details about the thing it needs to clean up).","commit_id":"6ba8a35825a7ec839b2d0aab7559351d573130ab"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"73ea162dd93143780fd64084ae7c864680f82663","unresolved":false,"context_lines":[{"line_number":13,"context_line":"with read_deleted\u003dyes."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: Ide6cc5bb1fce2c9aea9fa3efdf940e8308cd9ed0"},{"line_number":16,"context_line":"Closes-Bug: #1745977"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3fa0c359_130c9739","line":16,"in_reply_to":"3fa0c359_5821defa","updated":"2018-02-19 21:45:25.000000000","message":"I\u0027m not even sure why we need to lazy-load system_metadata since we should join that when we query the DB on the deleted instance:\n\nhttps://github.com/openstack/nova/blob/stable/pike/nova/db/sqlalchemy/api.py#L2141-L2143","commit_id":"6ba8a35825a7ec839b2d0aab7559351d573130ab"}],"nova/objects/instance.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"9be17bdcb7e4e2a1047fb08ec324fc39b16743e3","unresolved":false,"context_lines":[{"line_number":855,"context_line":"        self.obj_reset_changes()"},{"line_number":856,"context_line":""},{"line_number":857,"context_line":"    def _load_generic(self, attrname):"},{"line_number":858,"context_line":"        with utils.temporary_mutation(self._context, read_deleted\u003d\u0027yes\u0027):"},{"line_number":859,"context_line":"            instance \u003d self.__class__.get_by_uuid(self._context,"},{"line_number":860,"context_line":"                                                  uuid\u003dself.uuid,"},{"line_number":861,"context_line":"                                                  expected_attrs\u003d[attrname])"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_f3978aea","line":858,"updated":"2018-09-28 14:58:39.000000000","message":"Very, very late to the party here, and I realise that this seems to have become a pattern, but this is problematic when a RequestContext object is shared between threads, which we do pretty much everywhere we create worker threads. This is obviously going to mutate the object for all threads, which could potentially cause some very odd behaviour which would be very hard to debug.\n\nIt seems that outside of tests we only use temporary_mutation for setting read_deleted on a RequestContext, so I suggest that we replace it with a method like RequestContext.elevated() which returns a copy with read_deleted set. We should use that everywhere we currently use temporary_mutation, not just here. temporary_mutation should probably go away, or be moved into the test tree where it\u0027s probably easier to use without side-effects.","commit_id":"6ba8a35825a7ec839b2d0aab7559351d573130ab"}]}