)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":12732,"name":"Bin Zhou","email":"zhou.bin9@zte.com.cn","username":"BinZhou"},"change_message_id":"701aeeec66f542d569c746b3467db96bb8537c87","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Alvaro Lopez Garcia \u003caloga@ifca.unican.es\u003e"},{"line_number":5,"context_line":"CommitDate: 2016-06-20 08:02:52 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Ensure that periodic reclaim is able to clean DB deleted instances"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Under some circumstances, the _reclaim_queued_deletes tries to delete"},{"line_number":10,"context_line":"some instances that are partially deleted in the DB, failing with an"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"5a9d85d2_64227ebb","line":7,"range":{"start_line":7,"start_character":57,"end_line":7,"end_character":66},"updated":"2016-06-20 08:41:53.000000000","message":"The first line should be limited to 50 characters","commit_id":"457ea8911e643a9d3085fbd30cc316e7d6b27294"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"ef104a9b9ee3228cdac39f56ab48ddac0a05a85a","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Alvaro Lopez Garcia \u003caloga@ifca.unican.es\u003e"},{"line_number":5,"context_line":"CommitDate: 2016-06-20 08:02:52 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Ensure that periodic reclaim is able to clean DB deleted instances"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Under some circumstances, the _reclaim_queued_deletes tries to delete"},{"line_number":10,"context_line":"some instances that are partially deleted in the DB, failing with an"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"5a9d85d2_cfba6bc0","line":7,"range":{"start_line":7,"start_character":57,"end_line":7,"end_character":66},"in_reply_to":"5a9d85d2_64227ebb","updated":"2016-06-20 08:54:47.000000000","message":"I know that it _should_ be wrapped to 50 chars, but I think that readability is more important: I cannot condense the message into 50 chars (periodic + reclaim + deleted + instances is about 30 chars).","commit_id":"457ea8911e643a9d3085fbd30cc316e7d6b27294"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8157731ccc1f9ef55672773a9b9c6b5f71ef7bd6","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Under some circumstances, the _reclaim_queued_deletes tries to delete"},{"line_number":10,"context_line":"some instances that are partially deleted in the DB, failing with an"},{"line_number":11,"context_line":"InstanceNotFound error. The error persists until an operator manually"},{"line_number":12,"context_line":"removes the instance causing the exception. This change ensures that the"},{"line_number":13,"context_line":"_reclaim_queued_deletes method is getting the deleted instances as well"},{"line_number":14,"context_line":"so that the cleanup can be completed."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":24,"id":"1a6eadb0_1938bdee","line":11,"range":{"start_line":11,"start_character":0,"end_line":11,"end_character":16},"updated":"2016-12-15 15:35:18.000000000","message":"See my comment in the bug report, it\u0027s trying to lazy-load some values I think which needs to reload the entire instance from the database with some joined table probably, and because the instance is deleted we get the InstanceNotFound. If we loaded up those instances with the joined tables from the beginning, we might not hit that problem.","commit_id":"fe0f94b092e4922fed7e0ad0cc1dcd6297acbba3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c8c5a46d5cdb61788846a573b516ec59f0609bca","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Ensure that periodic reclaim cleans DB deleted instances"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Under some circumstances, the _reclaim_queued_deletes tries to delete"},{"line_number":10,"context_line":"some instances that are partially deleted in the DB, failing with an"},{"line_number":11,"context_line":"InstanceNotFound error. The error persists until an operator manually"},{"line_number":12,"context_line":"removes the instance causing the exception. This change ensures that the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":47,"id":"3fa7e38b_06a57647","line":9,"range":{"start_line":9,"start_character":0,"end_line":9,"end_character":24},"updated":"2019-10-22 19:48:38.000000000","message":"What circumstances? That\u0027s the question I have inline, i.e. how do I recreate this?","commit_id":"b698bbc79fba3a0a92d3019439501f2b520d5af9"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"3ceb0a1b0eb761641ebeb2418268f968bee51bd4","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Ensure that periodic reclaim cleans DB deleted instances"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Under some circumstances, the _reclaim_queued_deletes tries to delete"},{"line_number":10,"context_line":"some instances that are partially deleted in the DB, failing with an"},{"line_number":11,"context_line":"InstanceNotFound error. The error persists until an operator manually"},{"line_number":12,"context_line":"removes the instance causing the exception. This change ensures that the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":47,"id":"3fa7e38b_c16188aa","line":9,"range":{"start_line":9,"start_character":0,"end_line":9,"end_character":24},"in_reply_to":"3fa7e38b_06a57647","updated":"2019-10-22 20:33:21.000000000","message":"I do not know, but I have seen this in our production OpenStack (and in other deployments as well). For some unknown reason, sometimes, the instances were not properly deleted. I do not have the time to check why this is happening, and I cannot tell you how to reproduce this.\n\nHowever, as I said in the but report, I am abandoning this patchset as now I can consider the bug as a wont-fix (this is not happening anymore for new instances).","commit_id":"b698bbc79fba3a0a92d3019439501f2b520d5af9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"19742ff31f4e70e0d7742285c1183c1bb205ca6f","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Ensure that periodic reclaim cleans DB deleted instances"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Under some circumstances, the _reclaim_queued_deletes tries to delete"},{"line_number":10,"context_line":"some instances that are partially deleted in the DB, failing with an"},{"line_number":11,"context_line":"InstanceNotFound error. The error persists until an operator manually"},{"line_number":12,"context_line":"removes the instance causing the exception. This change ensures that the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":47,"id":"3fa7e38b_8115d04b","line":9,"range":{"start_line":9,"start_character":0,"end_line":9,"end_character":24},"in_reply_to":"3fa7e38b_c16188aa","updated":"2019-10-22 20:40:48.000000000","message":"\u003e However, as I said in the but report, I am abandoning this patchset\n \u003e as now I can consider the bug as a wont-fix (this is not happening\n \u003e anymore for new instances).\n\nOK, fair enough. I would not be surprised if there was maybe some bug where order was different and we would destroy the instance before updating the vm_state and the destroy worked in the DB but the vm_state update failed (intermittently of course, like if rabbit chokes or times out), and that was later fixed inadvertently by something else.","commit_id":"b698bbc79fba3a0a92d3019439501f2b520d5af9"}],"nova/compute/manager.py":[{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"081f44b5fb7aa73da557ca91a8c1c89fd04d7073","unresolved":false,"context_lines":[{"line_number":6243,"context_line":""},{"line_number":6244,"context_line":"        # NOTE(aloga): we need a context that is able to read deleted instances"},{"line_number":6245,"context_line":"        # but we cannot simply modify the context that is being passed to the"},{"line_number":6246,"context_line":"\t\t# method, as this is being reused for other periodic tasks that may"},{"line_number":6247,"context_line":"        # fail if they are getting deleted instances. We could modify the"},{"line_number":6248,"context_line":"        # incoming context to read_deleted\u003d\u0027yes\u0027 and then modify it back to"},{"line_number":6249,"context_line":"        # the original setting, but it seems safer to just get a new one."}],"source_content_type":"text/x-python","patch_set":1,"id":"9abb7d3a_680ca73b","line":6246,"updated":"2016-05-31 10:01:41.000000000","message":"OMG, vim has done something weird with tabs here.","commit_id":"12a94d020f2a97d3766d5506cabe1085d397a4b5"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"c69669f90c7a85a3d89311486fd6ff601d925d5d","unresolved":false,"context_lines":[{"line_number":6501,"context_line":"        # fail if they are getting deleted instances. We could modify the"},{"line_number":6502,"context_line":"        # incoming context to read_deleted\u003d\u0027yes\u0027 and then modify it back to"},{"line_number":6503,"context_line":"        # the original setting, but it seems safer to just get a new one."},{"line_number":6504,"context_line":"        context \u003d nova.context.get_admin_context(read_deleted\u003d\u0027yes\u0027)"},{"line_number":6505,"context_line":""},{"line_number":6506,"context_line":"        # TODO(comstud, jichenjc): Dummy quota object for now See bug 1296414."},{"line_number":6507,"context_line":"        # The only case that the quota might be inconsistent is"}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_97de963d","line":6504,"updated":"2016-12-14 15:02:08.000000000","message":"Nit: Its more normal to use the context manager like this one:\nhttps://github.com/openstack/nova/blob/master/nova/compute/manager.py#L6619","commit_id":"93b279d3d3c0930da21959daddd4a2ac3efc4fe2"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"7168e961ea12113ab2252dc0134287bff252c14c","unresolved":false,"context_lines":[{"line_number":6501,"context_line":"        # fail if they are getting deleted instances. We could modify the"},{"line_number":6502,"context_line":"        # incoming context to read_deleted\u003d\u0027yes\u0027 and then modify it back to"},{"line_number":6503,"context_line":"        # the original setting, but it seems safer to just get a new one."},{"line_number":6504,"context_line":"        context \u003d nova.context.get_admin_context(read_deleted\u003d\u0027yes\u0027)"},{"line_number":6505,"context_line":""},{"line_number":6506,"context_line":"        # TODO(comstud, jichenjc): Dummy quota object for now See bug 1296414."},{"line_number":6507,"context_line":"        # The only case that the quota might be inconsistent is"}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_41fb2ac1","line":6504,"in_reply_to":"1a6eadb0_97de963d","updated":"2016-12-15 12:45:03.000000000","message":"I was not aware of its existence, thanks for the pointer.","commit_id":"93b279d3d3c0930da21959daddd4a2ac3efc4fe2"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"c69669f90c7a85a3d89311486fd6ff601d925d5d","unresolved":false,"context_lines":[{"line_number":6517,"context_line":"        instances \u003d objects.InstanceList.get_by_filters("},{"line_number":6518,"context_line":"            context, filters,"},{"line_number":6519,"context_line":"            expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":6520,"context_line":"            use_slave\u003dTrue)"},{"line_number":6521,"context_line":"        for instance in instances:"},{"line_number":6522,"context_line":"            if self._deleted_old_enough(instance, interval):"},{"line_number":6523,"context_line":"                bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_77e1facf","line":6520,"updated":"2016-12-14 15:02:08.000000000","message":"I think we should actually put a limit on this query.\n\nIn an old cloud that has lots of failed soft_deleted cleanups, and they have not pruned their DB, this list could be quite big.\n\nI wouldn\u0027t mention it, but I have seen this cause OOM killing of nova-compute processes in a related situation in the past.","commit_id":"93b279d3d3c0930da21959daddd4a2ac3efc4fe2"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"7168e961ea12113ab2252dc0134287bff252c14c","unresolved":false,"context_lines":[{"line_number":6517,"context_line":"        instances \u003d objects.InstanceList.get_by_filters("},{"line_number":6518,"context_line":"            context, filters,"},{"line_number":6519,"context_line":"            expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":6520,"context_line":"            use_slave\u003dTrue)"},{"line_number":6521,"context_line":"        for instance in instances:"},{"line_number":6522,"context_line":"            if self._deleted_old_enough(instance, interval):"},{"line_number":6523,"context_line":"                bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_a19a3e6c","line":6520,"in_reply_to":"1a6eadb0_77e1facf","updated":"2016-12-15 12:45:03.000000000","message":"I do agree, I will put a number number here.","commit_id":"93b279d3d3c0930da21959daddd4a2ac3efc4fe2"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"c69669f90c7a85a3d89311486fd6ff601d925d5d","unresolved":false,"context_lines":[{"line_number":6523,"context_line":"                bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":6524,"context_line":"                        context, instance.uuid)"},{"line_number":6525,"context_line":"                LOG.info(_LI(\u0027Reclaiming deleted instance\u0027), instance\u003dinstance)"},{"line_number":6526,"context_line":"                try:"},{"line_number":6527,"context_line":"                    self._delete_instance(context, instance, bdms, quotas)"},{"line_number":6528,"context_line":"                except Exception as e:"},{"line_number":6529,"context_line":"                    LOG.warning(_LW(\"Periodic reclaim failed to delete \""}],"source_content_type":"text/x-python","patch_set":23,"id":"1a6eadb0_17fe0637","line":6526,"updated":"2016-12-14 15:02:08.000000000","message":"Nit: I would say its tempting for line 6523 to be put inside this try/except clause, in case that ever starts failing for a different reason, and kills the loop. But thats a separate change really.","commit_id":"93b279d3d3c0930da21959daddd4a2ac3efc4fe2"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8157731ccc1f9ef55672773a9b9c6b5f71ef7bd6","unresolved":false,"context_lines":[{"line_number":6516,"context_line":"                       \u0027host\u0027: self.host}"},{"line_number":6517,"context_line":"            # NOTE(aloga): use a limit here so that we do not get OOM if there"},{"line_number":6518,"context_line":"            # is a large number of soft-deleted instances that have failed."},{"line_number":6519,"context_line":"            instances \u003d objects.InstanceList.get_by_filters("},{"line_number":6520,"context_line":"                context, filters,"},{"line_number":6521,"context_line":"                expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":6522,"context_line":"                use_slave\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":24,"id":"1a6eadb0_990bed29","line":6519,"updated":"2016-12-15 15:35:18.000000000","message":"Where does it actually fail? The bug report isn\u0027t clear on this. Is it here? Or later?","commit_id":"fe0f94b092e4922fed7e0ad0cc1dcd6297acbba3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a092d807892725c56418230e5a71a422b5d83627","unresolved":false,"context_lines":[{"line_number":6516,"context_line":"                       \u0027host\u0027: self.host}"},{"line_number":6517,"context_line":"            # NOTE(aloga): use a limit here so that we do not get OOM if there"},{"line_number":6518,"context_line":"            # is a large number of soft-deleted instances that have failed."},{"line_number":6519,"context_line":"            instances \u003d objects.InstanceList.get_by_filters("},{"line_number":6520,"context_line":"                context, filters,"},{"line_number":6521,"context_line":"                expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":6522,"context_line":"                use_slave\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":24,"id":"1a6eadb0_22bd42b7","line":6519,"in_reply_to":"1a6eadb0_990bed29","updated":"2016-12-15 16:21:14.000000000","message":"aloga provided a better trace (this is liberty btw):\n\nhttp://paste.openstack.org/show/592513/\n\nSo this query is getting back deleted instances (instances.deleted value is !\u003d 0 in the database):\n\nhttps://github.com/openstack/nova/blob/liberty-eol/nova/db/sqlalchemy/api.py#L1891\n\nThat\u0027s because that query reads deleted instances by default, and apparently we have a deleted instances with the SOFT_DELETED vm_state somehow.\n\nAnyway, later on when we try to update the instance in this flow, to set the \u0027cleaned\u0027 attribute on the instance in the libvirt driver, it blows up with InstanceNotFound because the instance is actually deleted.\n\nI\u0027m not sure how the instances got into this state, but we probably do need to clean them up so that the volumes/networking/resource tracking can all do it\u0027s thing in this flow.\n\nI\u0027ll say I really don\u0027t like SOFT_DELETED as a feature, it\u0027s pretty gross and it\u0027s not tested in our CI system, but it is what it is I guess.","commit_id":"fe0f94b092e4922fed7e0ad0cc1dcd6297acbba3"},{"author":{"_account_id":6062,"name":"jichenjc","email":"jichenjc@cn.ibm.com","username":"jichenjc"},"change_message_id":"bba629df461ecc40ba163337f5e3fbd0cbfec30e","unresolved":false,"context_lines":[{"line_number":6524,"context_line":"                context, filters,"},{"line_number":6525,"context_line":"                expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":6526,"context_line":"                use_slave\u003dTrue,"},{"line_number":6527,"context_line":"                limit\u003d10)"},{"line_number":6528,"context_line":"            for instance in instances:"},{"line_number":6529,"context_line":"                if self._deleted_old_enough(instance, interval):"},{"line_number":6530,"context_line":"                    bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":28,"id":"ba5201f7_ca5b0bf5","line":6527,"range":{"start_line":6527,"start_character":16,"end_line":6527,"end_character":24},"updated":"2017-01-11 02:31:37.000000000","message":"didn\u0027t see OOM in the bug report, why this could lead to OOM?","commit_id":"56beab2f9d678e52c28b11c720aaab29b6d947ee"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"05941d3212f8749fc89948e1a1cde4cc284c4151","unresolved":false,"context_lines":[{"line_number":6524,"context_line":"                context, filters,"},{"line_number":6525,"context_line":"                expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":6526,"context_line":"                use_slave\u003dTrue,"},{"line_number":6527,"context_line":"                limit\u003d10)"},{"line_number":6528,"context_line":"            for instance in instances:"},{"line_number":6529,"context_line":"                if self._deleted_old_enough(instance, interval):"},{"line_number":6530,"context_line":"                    bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":28,"id":"ba5201f7_001cfdbf","line":6527,"range":{"start_line":6527,"start_character":16,"end_line":6527,"end_character":24},"in_reply_to":"ba5201f7_ca5b0bf5","updated":"2017-01-11 12:41:24.000000000","message":"See comment on previous patchset.","commit_id":"56beab2f9d678e52c28b11c720aaab29b6d947ee"},{"author":{"_account_id":15888,"name":"Zhenyu Zheng","email":"zheng.zhenyu@outlook.com","username":"Kevin_Zheng"},"change_message_id":"bb84b9615788781fc9db1a892dba9b78ac352ae0","unresolved":false,"context_lines":[{"line_number":7648,"context_line":"                       \u0027host\u0027: self.host}"},{"line_number":7649,"context_line":"            # NOTE(aloga): use a limit here so that we do not get OOM if there"},{"line_number":7650,"context_line":"            # is a large number of soft-deleted instances that have failed."},{"line_number":7651,"context_line":"            instances \u003d objects.InstanceList.get_by_filters("},{"line_number":7652,"context_line":"                context, filters,"},{"line_number":7653,"context_line":"                expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":7654,"context_line":"                use_slave\u003dTrue,"},{"line_number":7655,"context_line":"                limit\u003d10)"},{"line_number":7656,"context_line":"            for instance in instances:"},{"line_number":7657,"context_line":"                if self._deleted_old_enough(instance, interval):"},{"line_number":7658,"context_line":"                    bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":46,"id":"3f79a3b5_571571e6","line":7655,"range":{"start_line":7651,"start_character":12,"end_line":7655,"end_character":25},"updated":"2018-11-30 08:00:47.000000000","message":"should you add a loop here to reclaim all instances?","commit_id":"af81007cea620b8d1045360e2d862f0bddb26144"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"9d9c5f45aed8df702f4e191b841d4492a60093d7","unresolved":false,"context_lines":[{"line_number":7648,"context_line":"                       \u0027host\u0027: self.host}"},{"line_number":7649,"context_line":"            # NOTE(aloga): use a limit here so that we do not get OOM if there"},{"line_number":7650,"context_line":"            # is a large number of soft-deleted instances that have failed."},{"line_number":7651,"context_line":"            instances \u003d objects.InstanceList.get_by_filters("},{"line_number":7652,"context_line":"                context, filters,"},{"line_number":7653,"context_line":"                expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":7654,"context_line":"                use_slave\u003dTrue,"},{"line_number":7655,"context_line":"                limit\u003d10)"},{"line_number":7656,"context_line":"            for instance in instances:"},{"line_number":7657,"context_line":"                if self._deleted_old_enough(instance, interval):"},{"line_number":7658,"context_line":"                    bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":46,"id":"9fb8cfa7_8a63941a","line":7655,"range":{"start_line":7651,"start_character":12,"end_line":7655,"end_character":25},"in_reply_to":"3f79a3b5_571571e6","updated":"2019-06-06 09:07:19.000000000","message":"These will be reclaimed in the next iteration of the periodic task, therefore a loop is not needed.","commit_id":"af81007cea620b8d1045360e2d862f0bddb26144"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5cdb17fbd0aeee46fb939d5cdf7a47f78a08a65c","unresolved":false,"context_lines":[{"line_number":8048,"context_line":"                       \u0027host\u0027: self.host}"},{"line_number":8049,"context_line":"            # NOTE(aloga): use a limit here so that we do not get OOM if there"},{"line_number":8050,"context_line":"            # is a large number of soft-deleted instances that have failed."},{"line_number":8051,"context_line":"            instances \u003d objects.InstanceList.get_by_filters("},{"line_number":8052,"context_line":"                context, filters,"},{"line_number":8053,"context_line":"                expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":8054,"context_line":"                use_slave\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":47,"id":"3fa7e38b_c1d3c85e","line":8051,"updated":"2019-10-22 20:04:20.000000000","message":"Looking at the DB API for this (instance_get_all_by_filters_sort) it returns deleted instances by default:\n\nhttps://github.com/openstack/nova/blob/80385a22ee480a4c9775148d4729ab5d9c52e76d/nova/db/sqlalchemy/api.py#L2086\n\nWe could filter out deleted instances from the DB API query:\n\nhttps://github.com/openstack/nova/blob/80385a22ee480a4c9775148d4729ab5d9c52e76d/nova/db/sqlalchemy/api.py#L2183-L2208\n\nBy just adding deleted\u003dFalse to the filters dict above.\n\nWouldn\u0027t that be easier than messing with already-deleted instances? I still don\u0027t understand how we can get into a state where the instance is both deleted and SOFT_DELETED at the same time. Do you maybe have some external scripts/tooling that hard-deletes instances in the nova DB after they are soft deleted for some certain amount of time (effectively what this periodic is meant to do).","commit_id":"b698bbc79fba3a0a92d3019439501f2b520d5af9"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"3ceb0a1b0eb761641ebeb2418268f968bee51bd4","unresolved":false,"context_lines":[{"line_number":8048,"context_line":"                       \u0027host\u0027: self.host}"},{"line_number":8049,"context_line":"            # NOTE(aloga): use a limit here so that we do not get OOM if there"},{"line_number":8050,"context_line":"            # is a large number of soft-deleted instances that have failed."},{"line_number":8051,"context_line":"            instances \u003d objects.InstanceList.get_by_filters("},{"line_number":8052,"context_line":"                context, filters,"},{"line_number":8053,"context_line":"                expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":8054,"context_line":"                use_slave\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":47,"id":"3fa7e38b_81a07044","line":8051,"in_reply_to":"3fa7e38b_c1d3c85e","updated":"2019-10-22 20:33:21.000000000","message":"\u003e Wouldn\u0027t that be easier than messing with already-deleted\n \u003e instances? I still don\u0027t understand how we can get into a state\n \u003e where the instance is both deleted and SOFT_DELETED at the same\n \u003e time. Do you maybe have some external scripts/tooling that\n \u003e hard-deletes instances in the nova DB after they are soft deleted\n \u003e for some certain amount of time (effectively what this periodic is\n \u003e meant to do).\n\nBeing honest, 3 years later, I do not know. Maybe it was easier to go as you suggest, but probably something was wrong  (as I remember that this was not so straightforward), but who knows. After such a long time everything (including the DB) has changed.","commit_id":"b698bbc79fba3a0a92d3019439501f2b520d5af9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c8c5a46d5cdb61788846a573b516ec59f0609bca","unresolved":false,"context_lines":[{"line_number":8052,"context_line":"                context, filters,"},{"line_number":8053,"context_line":"                expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":8054,"context_line":"                use_slave\u003dTrue,"},{"line_number":8055,"context_line":"                limit\u003d10)"},{"line_number":8056,"context_line":"            for instance in instances:"},{"line_number":8057,"context_line":"                if self._deleted_old_enough(instance, interval):"},{"line_number":8058,"context_line":"                    bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":47,"id":"3fa7e38b_e6fb7a44","line":8055,"updated":"2019-10-22 19:48:38.000000000","message":"This is really a separate fix that shouldn\u0027t have anything to do with the deleted InstanceNotFound bug you\u0027re trying to fix. I\u0027d remove this from this change and fix it separately if necessary.","commit_id":"b698bbc79fba3a0a92d3019439501f2b520d5af9"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"3ceb0a1b0eb761641ebeb2418268f968bee51bd4","unresolved":false,"context_lines":[{"line_number":8052,"context_line":"                context, filters,"},{"line_number":8053,"context_line":"                expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":8054,"context_line":"                use_slave\u003dTrue,"},{"line_number":8055,"context_line":"                limit\u003d10)"},{"line_number":8056,"context_line":"            for instance in instances:"},{"line_number":8057,"context_line":"                if self._deleted_old_enough(instance, interval):"},{"line_number":8058,"context_line":"                    bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":47,"id":"3fa7e38b_8185d009","line":8055,"in_reply_to":"3fa7e38b_4607ee1e","updated":"2019-10-22 20:33:21.000000000","message":"This is what I most hate from code review, as I had to adjust this basically because somebody (John) requested it in the past.\n\nI reckon that the comming message should had been adjusted indicating it.","commit_id":"b698bbc79fba3a0a92d3019439501f2b520d5af9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"883225276f7c7f067a1f16b47da977e7aaea8b82","unresolved":false,"context_lines":[{"line_number":8052,"context_line":"                context, filters,"},{"line_number":8053,"context_line":"                expected_attrs\u003dobjects.instance.INSTANCE_DEFAULT_FIELDS,"},{"line_number":8054,"context_line":"                use_slave\u003dTrue,"},{"line_number":8055,"context_line":"                limit\u003d10)"},{"line_number":8056,"context_line":"            for instance in instances:"},{"line_number":8057,"context_line":"                if self._deleted_old_enough(instance, interval):"},{"line_number":8058,"context_line":"                    bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":47,"id":"3fa7e38b_4607ee1e","line":8055,"in_reply_to":"3fa7e38b_e6fb7a44","updated":"2019-10-22 19:53:29.000000000","message":"OK I found where this came from:\n\nhttps://review.opendev.org/#/c/323250/23/nova/compute/manager.py@6520\n\nThis should be called out in the commit message.","commit_id":"b698bbc79fba3a0a92d3019439501f2b520d5af9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c8c5a46d5cdb61788846a573b516ec59f0609bca","unresolved":false,"context_lines":[{"line_number":8059,"context_line":"                            context, instance.uuid)"},{"line_number":8060,"context_line":"                    LOG.info(\u0027Reclaiming deleted instance\u0027, instance\u003dinstance)"},{"line_number":8061,"context_line":"                    try:"},{"line_number":8062,"context_line":"                        self._delete_instance(context, instance, bdms)"},{"line_number":8063,"context_line":"                    except Exception as e:"},{"line_number":8064,"context_line":"                        LOG.warning(\"Periodic reclaim failed to delete \""},{"line_number":8065,"context_line":"                                    \"instance: %s\","}],"source_content_type":"text/x-python","patch_set":47,"id":"3fa7e38b_26b0b28b","line":8062,"updated":"2019-10-22 19:48:38.000000000","message":"If I\u0027m reading the stacktrace in the bug report correctly, the issue is that way down in the libvirt driver\u0027s cleanup method it updates the instance and saves it, which fails when the instance is (soft) deleted (not to be confused with being in the SOFT_DELETED state).\n\nYour fix here is to also read deleted SOFT_DELETED instances but I\u0027m not sure how it could be both deleted and SOFT_DELETED since once it\u0027s reclaimed the vm_state should change to DELETED (in the _delete_instance method) and that would mean it shouldn\u0027t come back in the filtered query above.\n\nSo how is it possible to get into this state?","commit_id":"b698bbc79fba3a0a92d3019439501f2b520d5af9"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"3ceb0a1b0eb761641ebeb2418268f968bee51bd4","unresolved":false,"context_lines":[{"line_number":8059,"context_line":"                            context, instance.uuid)"},{"line_number":8060,"context_line":"                    LOG.info(\u0027Reclaiming deleted instance\u0027, instance\u003dinstance)"},{"line_number":8061,"context_line":"                    try:"},{"line_number":8062,"context_line":"                        self._delete_instance(context, instance, bdms)"},{"line_number":8063,"context_line":"                    except Exception as e:"},{"line_number":8064,"context_line":"                        LOG.warning(\"Periodic reclaim failed to delete \""},{"line_number":8065,"context_line":"                                    \"instance: %s\","}],"source_content_type":"text/x-python","patch_set":47,"id":"3fa7e38b_2102dc3f","line":8062,"in_reply_to":"3fa7e38b_26b0b28b","updated":"2019-10-22 20:33:21.000000000","message":"I do not know. Our database does not have any external tooling that modifies it, therefore this was something caused by nova.","commit_id":"b698bbc79fba3a0a92d3019439501f2b520d5af9"}],"nova/tests/unit/compute/test_compute.py":[{"author":{"_account_id":6062,"name":"jichenjc","email":"jichenjc@cn.ibm.com","username":"jichenjc"},"change_message_id":"d9dc6b477b6933c69b07a3b7136a7eab95daceb4","unresolved":false,"context_lines":[{"line_number":7050,"context_line":"        # NOTE(aloga): the context used in the following calls is different"},{"line_number":7051,"context_line":"        # from the one that we have passed to the _reclaim_queued_deletes, as"},{"line_number":7052,"context_line":"        # the method gets a new context with \"read_deleted\u003dTrue\" so that it is"},{"line_number":7053,"context_line":"        # able to read deleted instances."},{"line_number":7054,"context_line":"        mock_get_filter.assert_called_once_with(mock.ANY, mock.ANY,"},{"line_number":7055,"context_line":"                expected_attrs\u003dinstance_obj.INSTANCE_DEFAULT_FIELDS,"},{"line_number":7056,"context_line":"                use_slave\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":16,"id":"9ad45d7e_0153f96b","line":7053,"range":{"start_line":7053,"start_character":40,"end_line":7053,"end_character":41},"updated":"2016-08-11 13:16:22.000000000","message":"maybe you can mock get_admin_context\nand use that value to avoid description here?","commit_id":"cab0475fe067a9593a940920d5f1468130344f6b"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"9d9c5f45aed8df702f4e191b841d4492a60093d7","unresolved":false,"context_lines":[{"line_number":7050,"context_line":"        # NOTE(aloga): the context used in the following calls is different"},{"line_number":7051,"context_line":"        # from the one that we have passed to the _reclaim_queued_deletes, as"},{"line_number":7052,"context_line":"        # the method gets a new context with \"read_deleted\u003dTrue\" so that it is"},{"line_number":7053,"context_line":"        # able to read deleted instances."},{"line_number":7054,"context_line":"        mock_get_filter.assert_called_once_with(mock.ANY, mock.ANY,"},{"line_number":7055,"context_line":"                expected_attrs\u003dinstance_obj.INSTANCE_DEFAULT_FIELDS,"},{"line_number":7056,"context_line":"                use_slave\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":16,"id":"9ad45d7e_6a0257b6","line":7053,"range":{"start_line":7053,"start_character":40,"end_line":7053,"end_character":41},"in_reply_to":"9ad45d7e_0153f96b","updated":"2019-06-06 09:07:19.000000000","message":"Indeed","commit_id":"cab0475fe067a9593a940920d5f1468130344f6b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8e797dbb3a6691962da010d3040a2e7b12ba36e3","unresolved":false,"context_lines":[{"line_number":7146,"context_line":"        mock_bdms.return_value \u003d []"},{"line_number":7147,"context_line":""},{"line_number":7148,"context_line":"        # Active"},{"line_number":7149,"context_line":"        self._create_fake_instance_obj(params\u003d{\u0027host\u0027: CONF.host})"},{"line_number":7150,"context_line":""},{"line_number":7151,"context_line":"        # Deleted not old enough"},{"line_number":7152,"context_line":"        self._create_fake_instance_obj(params\u003d{\u0027host\u0027: CONF.host,"}],"source_content_type":"text/x-python","patch_set":24,"id":"1a6eadb0_e2534a37","line":7149,"updated":"2016-12-15 16:26:04.000000000","message":"Seems this would be a better test if we actually recreated the scenario, which is create a 2nd instance, set it\u0027s vm_status to SOFT_DELETED *and* it\u0027s deleted value to instance.id (!\u003d 0 means deleted in the instances table). Then we\u0027d know if the fix is working.","commit_id":"fe0f94b092e4922fed7e0ad0cc1dcd6297acbba3"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"4e67b0cedec0485dd92b4fbbd639a5acc850b88b","unresolved":false,"context_lines":[{"line_number":7146,"context_line":"        mock_bdms.return_value \u003d []"},{"line_number":7147,"context_line":""},{"line_number":7148,"context_line":"        # Active"},{"line_number":7149,"context_line":"        self._create_fake_instance_obj(params\u003d{\u0027host\u0027: CONF.host})"},{"line_number":7150,"context_line":""},{"line_number":7151,"context_line":"        # Deleted not old enough"},{"line_number":7152,"context_line":"        self._create_fake_instance_obj(params\u003d{\u0027host\u0027: CONF.host,"}],"source_content_type":"text/x-python","patch_set":24,"id":"1a6eadb0_8ddac7e6","line":7149,"in_reply_to":"1a6eadb0_e2534a37","updated":"2016-12-15 16:48:01.000000000","message":"Ok, I will improve it.","commit_id":"fe0f94b092e4922fed7e0ad0cc1dcd6297acbba3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8e797dbb3a6691962da010d3040a2e7b12ba36e3","unresolved":false,"context_lines":[{"line_number":7206,"context_line":"        mock_delete_inst.side_effect \u003d (test.TestingException, None)"},{"line_number":7207,"context_line":"        mock_quota.return_value \u003d self.none_quotas"},{"line_number":7208,"context_line":""},{"line_number":7209,"context_line":"#        ctxt_delete \u003d context.get_admin_context(read_deleted\u003d\u0027yes\u0027)"},{"line_number":7210,"context_line":"#        with mock.patch.object(context,"},{"line_number":7211,"context_line":"#                               \u0027get_admin_context\u0027) as mock_get_context:"},{"line_number":7212,"context_line":"#            mock_get_context.return_value \u003d ctxt_delete"},{"line_number":7213,"context_line":"#"},{"line_number":7214,"context_line":"        self.compute._reclaim_queued_deletes(ctxt)"},{"line_number":7215,"context_line":""},{"line_number":7216,"context_line":"        mock_get_filter.assert_called_once_with(ctxt, mock.ANY,"}],"source_content_type":"text/x-python","patch_set":24,"id":"1a6eadb0_02f04666","line":7213,"range":{"start_line":7209,"start_character":0,"end_line":7213,"end_character":1},"updated":"2016-12-15 16:26:04.000000000","message":"This needs to be cleaned up, you can\u0027t leave commented out code in the tests.","commit_id":"fe0f94b092e4922fed7e0ad0cc1dcd6297acbba3"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"4e67b0cedec0485dd92b4fbbd639a5acc850b88b","unresolved":false,"context_lines":[{"line_number":7206,"context_line":"        mock_delete_inst.side_effect \u003d (test.TestingException, None)"},{"line_number":7207,"context_line":"        mock_quota.return_value \u003d self.none_quotas"},{"line_number":7208,"context_line":""},{"line_number":7209,"context_line":"#        ctxt_delete \u003d context.get_admin_context(read_deleted\u003d\u0027yes\u0027)"},{"line_number":7210,"context_line":"#        with mock.patch.object(context,"},{"line_number":7211,"context_line":"#                               \u0027get_admin_context\u0027) as mock_get_context:"},{"line_number":7212,"context_line":"#            mock_get_context.return_value \u003d ctxt_delete"},{"line_number":7213,"context_line":"#"},{"line_number":7214,"context_line":"        self.compute._reclaim_queued_deletes(ctxt)"},{"line_number":7215,"context_line":""},{"line_number":7216,"context_line":"        mock_get_filter.assert_called_once_with(ctxt, mock.ANY,"}],"source_content_type":"text/x-python","patch_set":24,"id":"1a6eadb0_8d53a76a","line":7213,"range":{"start_line":7209,"start_character":0,"end_line":7213,"end_character":1},"in_reply_to":"1a6eadb0_02f04666","updated":"2016-12-15 16:48:01.000000000","message":"OMG, this should not be there :(","commit_id":"fe0f94b092e4922fed7e0ad0cc1dcd6297acbba3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8e797dbb3a6691962da010d3040a2e7b12ba36e3","unresolved":false,"context_lines":[{"line_number":7216,"context_line":"        mock_get_filter.assert_called_once_with(ctxt, mock.ANY,"},{"line_number":7217,"context_line":"                expected_attrs\u003dinstance_obj.INSTANCE_DEFAULT_FIELDS,"},{"line_number":7218,"context_line":"                use_slave\u003dTrue, limit\u003d10)"},{"line_number":7219,"context_line":"        mock_delete_old.assert_has_calls(["},{"line_number":7220,"context_line":"            mock.call(instance1, 3600),"},{"line_number":7221,"context_line":"            mock.call(instance2, 3600)])"},{"line_number":7222,"context_line":"        mock_get_uuid.assert_has_calls(["},{"line_number":7223,"context_line":"            mock.call(ctxt, instance1.uuid),"},{"line_number":7224,"context_line":"            mock.call(ctxt, instance2.uuid)])"},{"line_number":7225,"context_line":"        mock_delete_inst.assert_has_calls(["},{"line_number":7226,"context_line":"            mock.call(ctxt, instance1, [], self.none_quotas),"},{"line_number":7227,"context_line":"            mock.call(ctxt, instance2, [], self.none_quotas)])"}],"source_content_type":"text/x-python","patch_set":24,"id":"1a6eadb0_c2face42","line":7224,"range":{"start_line":7219,"start_character":8,"end_line":7224,"end_character":45},"updated":"2016-12-15 16:26:04.000000000","message":"You didn\u0027t actually need to touch these lines.","commit_id":"fe0f94b092e4922fed7e0ad0cc1dcd6297acbba3"},{"author":{"_account_id":91,"name":"Alvaro","email":"aloga@ifca.unican.es","username":"aloga"},"change_message_id":"4e67b0cedec0485dd92b4fbbd639a5acc850b88b","unresolved":false,"context_lines":[{"line_number":7216,"context_line":"        mock_get_filter.assert_called_once_with(ctxt, mock.ANY,"},{"line_number":7217,"context_line":"                expected_attrs\u003dinstance_obj.INSTANCE_DEFAULT_FIELDS,"},{"line_number":7218,"context_line":"                use_slave\u003dTrue, limit\u003d10)"},{"line_number":7219,"context_line":"        mock_delete_old.assert_has_calls(["},{"line_number":7220,"context_line":"            mock.call(instance1, 3600),"},{"line_number":7221,"context_line":"            mock.call(instance2, 3600)])"},{"line_number":7222,"context_line":"        mock_get_uuid.assert_has_calls(["},{"line_number":7223,"context_line":"            mock.call(ctxt, instance1.uuid),"},{"line_number":7224,"context_line":"            mock.call(ctxt, instance2.uuid)])"},{"line_number":7225,"context_line":"        mock_delete_inst.assert_has_calls(["},{"line_number":7226,"context_line":"            mock.call(ctxt, instance1, [], self.none_quotas),"},{"line_number":7227,"context_line":"            mock.call(ctxt, instance2, [], self.none_quotas)])"}],"source_content_type":"text/x-python","patch_set":24,"id":"1a6eadb0_edc89b24","line":7224,"range":{"start_line":7219,"start_character":8,"end_line":7224,"end_character":45},"in_reply_to":"1a6eadb0_c2face42","updated":"2016-12-15 16:48:01.000000000","message":"Yes, these are leftovers from the previous patchset.","commit_id":"fe0f94b092e4922fed7e0ad0cc1dcd6297acbba3"}]}