)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"03cb55a4e09c9a563559b21f5c7a55a30672ee40","unresolved":false,"context_lines":[{"line_number":13,"context_line":"But for anything else, we should allow reading that data if it\u0027s still there."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"With this change, we are able to remove a specific read_deleted\u003d\u0027yes\u0027 usage"},{"line_number":16,"context_line":"from ComputeManager._destroy_evacuated_instances() which is handled with"},{"line_number":17,"context_line":"the generic solution. TestEvacuateDeleteServerRestartOriginalCompute asserts"},{"line_number":18,"context_line":"that the evacuate scenario is still fixed."},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"3f79a3b5_b6cc44be","line":16,"updated":"2018-11-27 19:18:37.000000000","message":"...introduced via https://review.openstack.org/#/c/606122/...","commit_id":"604819b29c0bd43969747d32f6e3d818b3cbece7"}],"nova/objects/instance.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"afc854f0d1613370527ae8a4d44aadd811e53134","unresolved":false,"context_lines":[{"line_number":1096,"context_line":"        with utils.temporary_mutation(self._context, read_deleted\u003d\u0027yes\u0027):"},{"line_number":1097,"context_line":"            self._obj_load_attr_deleted(attrname)"},{"line_number":1098,"context_line":""},{"line_number":1099,"context_line":"    def _obj_load_attr_deleted(self, attrname):"},{"line_number":1100,"context_line":"        # NOTE(danms): We handle some fields differently here so that we"},{"line_number":1101,"context_line":"        # can be more efficient"},{"line_number":1102,"context_line":"        if attrname \u003d\u003d \u0027fault\u0027:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_cb7ac7ed","line":1099,"range":{"start_line":1099,"start_character":23,"end_line":1099,"end_character":30},"updated":"2018-10-24 12:57:17.000000000","message":"This is a bit confusing if it\u0027s ever called by something that doesn\u0027t have self._context.read_deleted set to \u0027yes\u0027. I mean, it\u0027s named this way because of how it\u0027s being called, but there isn\u0027t really a restriction that it must be called that way. If you leave the name as this, then I\u0027d suggest at least adding a docstring for usage. Or just call it _obj_load_attr().","commit_id":"9671cd4daab37a2f562e4d20eb45899954650530"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"d6f6f38fc4e93f3a8911deb1704df8757da58e76","unresolved":false,"context_lines":[{"line_number":897,"context_line":"        self.obj_reset_changes()"},{"line_number":898,"context_line":""},{"line_number":899,"context_line":"    def _load_generic(self, attrname):"},{"line_number":900,"context_line":"        with utils.temporary_mutation(self._context, read_deleted\u003d\u0027yes\u0027):"},{"line_number":901,"context_line":"            instance \u003d self.__class__.get_by_uuid(self._context,"},{"line_number":902,"context_line":"                                                  uuid\u003dself.uuid,"},{"line_number":903,"context_line":"                                                  expected_attrs\u003d[attrname])"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_f4f0a9dc","side":"PARENT","line":900,"updated":"2018-10-25 12:17:54.000000000","message":"Ack. This is called from obj_load_attr, which now sets this for us.","commit_id":"835faf3f40af6b0e07c585690982a997d6a2ac47"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"d6f6f38fc4e93f3a8911deb1704df8757da58e76","unresolved":false,"context_lines":[{"line_number":982,"context_line":""},{"line_number":983,"context_line":"    def _load_security_groups(self):"},{"line_number":984,"context_line":"        self.security_groups \u003d objects.SecurityGroupList.get_by_instance("},{"line_number":985,"context_line":"            self._context, self)"},{"line_number":986,"context_line":""},{"line_number":987,"context_line":"    def _load_pci_devices(self):"},{"line_number":988,"context_line":"        self.pci_devices \u003d objects.PciDeviceList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_00eba0a4","line":985,"updated":"2018-10-25 12:17:54.000000000","message":"I think we\u0027ve introduced a bug here: with this change we\u0027ll now return deleted security groups where previously we didn\u0027t.\n\nI don\u0027t think we actually care about this, as IIUC we\u0027re not using these any more (nova network only?). However, we should do one of:\n\n* Not break it\n* Remove it if we\u0027re really not using it\n* Call out explicitly that we\u0027re breaking it but we don\u0027t care\n\nI\u0027d personally go for either 2 or 3. 3 is obviously smaller, 2 could be a follow-up.","commit_id":"dcf41206c8baee3c2799e457e5eb6ac18f7c6004"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8926ced7f443ab4c6046cb185be7797c40aa252d","unresolved":false,"context_lines":[{"line_number":982,"context_line":""},{"line_number":983,"context_line":"    def _load_security_groups(self):"},{"line_number":984,"context_line":"        self.security_groups \u003d objects.SecurityGroupList.get_by_instance("},{"line_number":985,"context_line":"            self._context, self)"},{"line_number":986,"context_line":""},{"line_number":987,"context_line":"    def _load_pci_devices(self):"},{"line_number":988,"context_line":"        self.pci_devices \u003d objects.PciDeviceList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_64e14197","line":985,"in_reply_to":"3f79a3b5_00eba0a4","updated":"2018-10-25 16:09:06.000000000","message":"\u003e I think we\u0027ve introduced a bug here: with this change we\u0027ll now return deleted security groups where previously we didn\u0027t.\n\nIsn\u0027t this true of anything that gets lazy-loaded from an instance object now? I was thinking about the instance.services record because I know we\u0027ve had issues with that in the past, see bug 1764556 and bug 1778305. The former might actually have already been fixed, or would be fixed by this. But I\u0027m not sure about the latter - would have to ask melwitt since she has the (now old) patches for that bug.","commit_id":"dcf41206c8baee3c2799e457e5eb6ac18f7c6004"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c1faf93dc2cc9c6ead1cf4c393458681c44a0893","unresolved":false,"context_lines":[{"line_number":982,"context_line":""},{"line_number":983,"context_line":"    def _load_security_groups(self):"},{"line_number":984,"context_line":"        self.security_groups \u003d objects.SecurityGroupList.get_by_instance("},{"line_number":985,"context_line":"            self._context, self)"},{"line_number":986,"context_line":""},{"line_number":987,"context_line":"    def _load_pci_devices(self):"},{"line_number":988,"context_line":"        self.pci_devices \u003d objects.PciDeviceList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_cfab6c6d","line":985,"in_reply_to":"3f79a3b5_165671ce","updated":"2018-11-27 16:56:42.000000000","message":"Matt has explained what he meant to me twice outside of this review and I keep forgetting, so I\u0027m putting this here for posterity:\n\nThe concern is a not-deleted instance would now be reading deleted security groups in addition to not-deleted ones. There are two reasons for which this isn\u0027t a concern:\n\n1. Security groups in our DB are only for nova-net, which is only supported with cellsv1 these days. Cellsv1 doesn\u0027t support security groups at all, and thus there\u0027s really no supportable case where security groups can be present today.\n\n2. The sqlalchemy api specifically excludes deleted groups in its low-level query (in that it doesn\u0027t honor the read_deleted flag in the context), so any deleted groups will never be read under any circumstance.","commit_id":"dcf41206c8baee3c2799e457e5eb6ac18f7c6004"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8230df19c30e4428bbbb477a1795d50e767e9f79","unresolved":false,"context_lines":[{"line_number":982,"context_line":""},{"line_number":983,"context_line":"    def _load_security_groups(self):"},{"line_number":984,"context_line":"        self.security_groups \u003d objects.SecurityGroupList.get_by_instance("},{"line_number":985,"context_line":"            self._context, self)"},{"line_number":986,"context_line":""},{"line_number":987,"context_line":"    def _load_pci_devices(self):"},{"line_number":988,"context_line":"        self.pci_devices \u003d objects.PciDeviceList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_0f8de427","line":985,"in_reply_to":"3f79a3b5_165671ce","updated":"2018-11-27 16:53:30.000000000","message":"SecurityGroupList.get_by_instance calls this:\n\nhttps://github.com/openstack/nova/blob/8545ba2af7476e0884b5e7fb90965bef92d605bc/nova/db/sqlalchemy/api.py#L3975\n\nWhich hard-codes read_deleted\u003d\u0027no\u0027 so we wouldn\u0027t ever return deleted security groups from this, as far as I can tell, regardless of us mutating the context.read_deleted\u003d\u0027yes\u0027 during lazy loads.","commit_id":"dcf41206c8baee3c2799e457e5eb6ac18f7c6004"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"28f4c6cc5f31a24427b4b5795cbddaca8180e476","unresolved":false,"context_lines":[{"line_number":982,"context_line":""},{"line_number":983,"context_line":"    def _load_security_groups(self):"},{"line_number":984,"context_line":"        self.security_groups \u003d objects.SecurityGroupList.get_by_instance("},{"line_number":985,"context_line":"            self._context, self)"},{"line_number":986,"context_line":""},{"line_number":987,"context_line":"    def _load_pci_devices(self):"},{"line_number":988,"context_line":"        self.pci_devices \u003d objects.PciDeviceList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_d7b6c526","line":985,"in_reply_to":"3f79a3b5_64e14197","updated":"2018-10-25 16:34:39.000000000","message":"I don\u0027t think bug 1764556 and bug 1778305 have been fixed or will be fixed by this. Those bugs are about service UUID generation failing for older, deleted services in people\u0027s databases during an upgrade. Without the change in this patch, we\u0027re already loading deleted services via _load_generic because _load_generic uses read_deleted\u003d\u0027yes\u0027. This patch makes us read even more deleted things, which will not change the behavior of reading deleted services. And that\u0027s cool.\n\nMy patch fix for the aforementioned bugs is to not generate a UUID for a UUID-less service, if it is deleted. We still need that fix and it\u0027s not related to this patch.","commit_id":"dcf41206c8baee3c2799e457e5eb6ac18f7c6004"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c840eebb3b768850ae68de6d7af543d0d4b77ba9","unresolved":false,"context_lines":[{"line_number":982,"context_line":""},{"line_number":983,"context_line":"    def _load_security_groups(self):"},{"line_number":984,"context_line":"        self.security_groups \u003d objects.SecurityGroupList.get_by_instance("},{"line_number":985,"context_line":"            self._context, self)"},{"line_number":986,"context_line":""},{"line_number":987,"context_line":"    def _load_pci_devices(self):"},{"line_number":988,"context_line":"        self.pci_devices \u003d objects.PciDeviceList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_a4b9997c","line":985,"in_reply_to":"3f79a3b5_64e14197","updated":"2018-10-25 16:14:06.000000000","message":"Yes, and it\u0027s no different if we use an alternate context when fetching an instance and loading or lazy-loading attributes. For the security groups thing specifically, I think this is effectively empty now for all supported cases and thus not really something we need to worry about here. If we want to remove the loading of secgroups to ensure that, then fine, but that is a different patch I think.","commit_id":"dcf41206c8baee3c2799e457e5eb6ac18f7c6004"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"a38a0b22390544aae1c6bf2b8f5b88570e7fb50e","unresolved":false,"context_lines":[{"line_number":982,"context_line":""},{"line_number":983,"context_line":"    def _load_security_groups(self):"},{"line_number":984,"context_line":"        self.security_groups \u003d objects.SecurityGroupList.get_by_instance("},{"line_number":985,"context_line":"            self._context, self)"},{"line_number":986,"context_line":""},{"line_number":987,"context_line":"    def _load_pci_devices(self):"},{"line_number":988,"context_line":"        self.pci_devices \u003d objects.PciDeviceList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_165671ce","line":985,"in_reply_to":"3f79a3b5_d7b6c526","updated":"2018-10-26 09:08:07.000000000","message":"I did a quick audit of all the field loads in this method. I don\u0027t think any others are affected, as they all have other guards in place which prevent loading a deleted object, mostly having \u0027deleted \u003d\u003d 0\u0027 on the SqlA relationship in models. This was the only load which loaded the target object directly and didn\u0027t have specific handling.","commit_id":"dcf41206c8baee3c2799e457e5eb6ac18f7c6004"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"d6f6f38fc4e93f3a8911deb1704df8757da58e76","unresolved":false,"context_lines":[{"line_number":1108,"context_line":"                   \u0027uuid\u0027: self.uuid,"},{"line_number":1109,"context_line":"                   })"},{"line_number":1110,"context_line":""},{"line_number":1111,"context_line":"        with utils.temporary_mutation(self._context, read_deleted\u003d\u0027yes\u0027):"},{"line_number":1112,"context_line":"            self._obj_load_attr(attrname)"},{"line_number":1113,"context_line":""},{"line_number":1114,"context_line":"    def _obj_load_attr(self, attrname):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_94aff5df","line":1111,"updated":"2018-10-25 12:17:54.000000000","message":"Noting here really just to raise awareness of a thing: this is just code motion here so it\u0027s not introducing a new bug. It does slightly increase the scope of the bug making it slightly more likely to race, but I doubt that\u0027s significant.\n\nutils.temporary_mutation(context) is a bad pattern because it modifies an object which can be shared between threads of execution. For example, it\u0027s shared across all threads spawned by _do_build_and_run_instance, live migration and its monitor, and all threads spawned by sync_power_states. Probably more, this was just a cursory search for \u0027spawn_n\u0027. Especially with something like lazy loading, it\u0027s really hard to guarantee that we\u0027re never going to end up here while the context object is in use elsewhere. Even if we can it\u0027s a large, ongoing, non-obvious mental effort which, if we forget it, could result in some seriously bizarre bugs. We should think about refactoring all uses of utils.temporary_mutation(context) to something which returns a copy.","commit_id":"dcf41206c8baee3c2799e457e5eb6ac18f7c6004"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"a38a0b22390544aae1c6bf2b8f5b88570e7fb50e","unresolved":false,"context_lines":[{"line_number":1108,"context_line":"                   \u0027uuid\u0027: self.uuid,"},{"line_number":1109,"context_line":"                   })"},{"line_number":1110,"context_line":""},{"line_number":1111,"context_line":"        with utils.temporary_mutation(self._context, read_deleted\u003d\u0027yes\u0027):"},{"line_number":1112,"context_line":"            self._obj_load_attr(attrname)"},{"line_number":1113,"context_line":""},{"line_number":1114,"context_line":"    def _obj_load_attr(self, attrname):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_36b92deb","line":1111,"in_reply_to":"3f79a3b5_6464e120","updated":"2018-10-26 09:08:07.000000000","message":"Yeah, I don\u0027t think it\u0027s necessary to change here. I just think it would be good to phase it out everywhere over time, especially as the code change from context manager to copy is trivial. It\u0027s one of those things whose potential interactions are large and unpredictable: it would be simpler to reason about Nova in general if it didn\u0027t exist.","commit_id":"dcf41206c8baee3c2799e457e5eb6ac18f7c6004"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c840eebb3b768850ae68de6d7af543d0d4b77ba9","unresolved":false,"context_lines":[{"line_number":1108,"context_line":"                   \u0027uuid\u0027: self.uuid,"},{"line_number":1109,"context_line":"                   })"},{"line_number":1110,"context_line":""},{"line_number":1111,"context_line":"        with utils.temporary_mutation(self._context, read_deleted\u003d\u0027yes\u0027):"},{"line_number":1112,"context_line":"            self._obj_load_attr(attrname)"},{"line_number":1113,"context_line":""},{"line_number":1114,"context_line":"    def _obj_load_attr(self, attrname):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f79a3b5_6464e120","line":1111,"in_reply_to":"3f79a3b5_94aff5df","updated":"2018-10-25 16:14:06.000000000","message":"This is a common pattern in nova (that temporary_mutation helper has existed for a long time). We know it has implications, but I\u0027m not sure it really matters that much for this case. Many of the spawn() things you\u0027re talking about would be stopped in their tracks once an instance is deleted anyway.\n\nI can use an obj_alternate_context() level in here with a cloned context if people want, which will avoid mutating the one we\u0027ve got here.","commit_id":"dcf41206c8baee3c2799e457e5eb6ac18f7c6004"}]}