)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"60ea3b219ecb8a2ca7ab45be2fc848ea06219e5c","unresolved":false,"context_lines":[{"line_number":11,"context_line":"field containing the project_id for the instance. The user_id field"},{"line_number":12,"context_line":"will contain the corresponding user_id for the instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The field should ideally be non-nullable, but we need to handle the"},{"line_number":15,"context_line":"fact that already existing instance mappings, that have not yet"},{"line_number":16,"context_line":"scheduled the instance to a cell, will be unable to be migrated to"},{"line_number":17,"context_line":"populate user_id until they are scheduled. We need the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9fdfeff1_0b6df501","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":40},"updated":"2019-02-27 19:07:10.000000000","message":"This is true but only because of existing instance mappings for existing instances that have not yet been migrated.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"7273ab4598fa8a58b3871d4c3a5549f2e329cca2","unresolved":false,"context_lines":[{"line_number":11,"context_line":"field containing the project_id for the instance. The user_id field"},{"line_number":12,"context_line":"will contain the corresponding user_id for the instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The field should ideally be non-nullable, but we need to handle the"},{"line_number":15,"context_line":"fact that already existing instance mappings, that have not yet"},{"line_number":16,"context_line":"scheduled the instance to a cell, will be unable to be migrated to"},{"line_number":17,"context_line":"populate user_id until they are scheduled. We need the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9fdfeff1_c6512464","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":40},"in_reply_to":"9fdfeff1_0b6df501","updated":"2019-02-27 19:50:13.000000000","message":"And because of existing instance mappings for existing instances that have not yet been scheduled (if their request specs are older than Rocky). But maybe that doesn\u0027t really make sense (see more comments below).","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"230ee439122b3021ada5cb1062dd34daa7b27ef9","unresolved":false,"context_lines":[{"line_number":11,"context_line":"field containing the project_id for the instance. The user_id field"},{"line_number":12,"context_line":"will contain the corresponding user_id for the instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The field should ideally be non-nullable, but we need to handle the"},{"line_number":15,"context_line":"fact that already existing instance mappings, that have not yet"},{"line_number":16,"context_line":"scheduled the instance to a cell, will be unable to be migrated to"},{"line_number":17,"context_line":"populate user_id until they are scheduled. We need the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9fdfeff1_7647c14d","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":40},"in_reply_to":"9fdfeff1_8c4c9b3f","updated":"2019-02-27 22:41:22.000000000","message":"Yes, was talking about the tight window during an upgrade.\n\nJust to add a note here in the review: we\u0027ve since discussed this in detail on IRC today, so I\u0027m going to rework the nova-manage migration to not let instances with the tight window situation through.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"02e6532bc994d8d2a923cdd9036061e278a302ba","unresolved":false,"context_lines":[{"line_number":11,"context_line":"field containing the project_id for the instance. The user_id field"},{"line_number":12,"context_line":"will contain the corresponding user_id for the instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The field should ideally be non-nullable, but we need to handle the"},{"line_number":15,"context_line":"fact that already existing instance mappings, that have not yet"},{"line_number":16,"context_line":"scheduled the instance to a cell, will be unable to be migrated to"},{"line_number":17,"context_line":"populate user_id until they are scheduled. We need the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9fdfeff1_8c4c9b3f","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":40},"in_reply_to":"9fdfeff1_c6512464","updated":"2019-02-27 20:34:06.000000000","message":"\u003e And because of existing instance mappings for existing instances\n \u003e that have not yet been scheduled (if their request specs are older\n \u003e than Rocky). But maybe that doesn\u0027t really make sense (see more\n \u003e comments below).\n\nWhen you say \"not yet scheduled\" do mean instance mappings created with an old API but with new conductor code? i.e. a very tight window during an upgrade? I wouldn\u0027t worry about that, as I said in the next change in the series. Worry about setting the value at create time and opportunistically on normal GET but I don\u0027t think we need to be paranoid about that upgrade window. The online data migration will handle the rest.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"60ea3b219ecb8a2ca7ab45be2fc848ea06219e5c","unresolved":false,"context_lines":[{"line_number":12,"context_line":"will contain the corresponding user_id for the instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The field should ideally be non-nullable, but we need to handle the"},{"line_number":15,"context_line":"fact that already existing instance mappings, that have not yet"},{"line_number":16,"context_line":"scheduled the instance to a cell, will be unable to be migrated to"},{"line_number":17,"context_line":"populate user_id until they are scheduled. We need the"},{"line_number":18,"context_line":"instance.user_id from the cell to know what user_id to set."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"We expect callers of InstanceMapping.create to have specified user_id"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9fdfeff1_ab7709d4","line":17,"range":{"start_line":15,"start_character":46,"end_line":17,"end_character":43},"updated":"2019-02-27 19:07:10.000000000","message":"This is not true. See below.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"7273ab4598fa8a58b3871d4c3a5549f2e329cca2","unresolved":false,"context_lines":[{"line_number":12,"context_line":"will contain the corresponding user_id for the instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The field should ideally be non-nullable, but we need to handle the"},{"line_number":15,"context_line":"fact that already existing instance mappings, that have not yet"},{"line_number":16,"context_line":"scheduled the instance to a cell, will be unable to be migrated to"},{"line_number":17,"context_line":"populate user_id until they are scheduled. We need the"},{"line_number":18,"context_line":"instance.user_id from the cell to know what user_id to set."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"We expect callers of InstanceMapping.create to have specified user_id"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9fdfeff1_915b5420","line":17,"range":{"start_line":15,"start_character":46,"end_line":17,"end_character":43},"in_reply_to":"9fdfeff1_ab7709d4","updated":"2019-02-27 19:50:13.000000000","message":"It thought it is true, as I mentioned, for \"already existing instance mappings\" that were created on the old side of an upgrade, or even older previous versions.\n\nSince you pointed out that RequestSpec.user_id is set in the API, that makes it sound like we could just use request spec to populate user_id for old instance mappings. I was informed RequestSpec.user_id was added recently, in Rocky [1], so I was thinking we can\u0027t rely on RequestSpec.user_id being set for request specs older than Rocky.\n\nBut, if the gap we\u0027re trying to account for is old instance mappings yet-to-be-scheduled, maybe it doesn\u0027t make sense that they could have request specs older than Rocky. \n\n[1] https://github.com/openstack/nova/commit/6e49019fae80586c4bbb8a7281600cf6140c176a","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"c05b4ac7ef5784e84078986df136ca79e4a64169","unresolved":false,"context_lines":[{"line_number":15,"context_line":"fact that already existing instance mappings, that have not yet"},{"line_number":16,"context_line":"scheduled the instance to a cell, will be unable to be migrated to"},{"line_number":17,"context_line":"populate user_id until they are scheduled. We need the"},{"line_number":18,"context_line":"instance.user_id from the cell to know what user_id to set."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"We expect callers of InstanceMapping.create to have specified user_id"},{"line_number":21,"context_line":"when creating an instance mapping, but if they do not, we default"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9fdfeff1_c4429b3b","line":18,"updated":"2019-02-26 22:51:30.000000000","message":"this is a little odd to me... isn\u0027t it possible to get the user ID from the build request, which would avoid needing to hit the cell at all? That said, I\u0027ll address in following patches :)","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"7273ab4598fa8a58b3871d4c3a5549f2e329cca2","unresolved":false,"context_lines":[{"line_number":15,"context_line":"fact that already existing instance mappings, that have not yet"},{"line_number":16,"context_line":"scheduled the instance to a cell, will be unable to be migrated to"},{"line_number":17,"context_line":"populate user_id until they are scheduled. We need the"},{"line_number":18,"context_line":"instance.user_id from the cell to know what user_id to set."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"We expect callers of InstanceMapping.create to have specified user_id"},{"line_number":21,"context_line":"when creating an instance mapping, but if they do not, we default"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9fdfeff1_e6b46884","line":18,"in_reply_to":"9fdfeff1_2bbf9961","updated":"2019-02-27 19:50:13.000000000","message":"\u003e Huh? The user_id is on the context, we have that in the API. We\n \u003e shouldn\u0027t need to know the cell to know the user_id, because it\u0027s\n \u003e in the context.\n \u003e \n \u003e In the API, the RequestSpec.project_id and user_id get set by this\n \u003e call:\n \u003e \n \u003e https://github.com/openstack/nova/blob/9c024e45bb6bd66a73e6cce8b739455799ff564c/nova/compute/api.py#L886\n \u003e \n \u003e https://github.com/openstack/nova/blob/9c024e45bb6bd66a73e6cce8b739455799ff564c/nova/objects/request_spec.py#L463-L464\n\nAh, thanks. I missed where it\u0027s set in the API. The only setting I had found previously was \u0027ensure_project_and_user_id\u0027 in conductor.\n\n \u003e Therefore we can definitely set the InstanceMapping.user_id here as\n \u003e well:\n \u003e \n \u003e https://github.com/openstack/nova/blob/9c024e45bb6bd66a73e6cce8b739455799ff564c/nova/compute/api.py#L949\n\nI\u0027m already setting it there.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"60ea3b219ecb8a2ca7ab45be2fc848ea06219e5c","unresolved":false,"context_lines":[{"line_number":15,"context_line":"fact that already existing instance mappings, that have not yet"},{"line_number":16,"context_line":"scheduled the instance to a cell, will be unable to be migrated to"},{"line_number":17,"context_line":"populate user_id until they are scheduled. We need the"},{"line_number":18,"context_line":"instance.user_id from the cell to know what user_id to set."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"We expect callers of InstanceMapping.create to have specified user_id"},{"line_number":21,"context_line":"when creating an instance mapping, but if they do not, we default"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9fdfeff1_2bbf9961","line":18,"in_reply_to":"9fdfeff1_53a50b02","updated":"2019-02-27 19:07:10.000000000","message":"Huh? The user_id is on the context, we have that in the API. We shouldn\u0027t need to know the cell to know the user_id, because it\u0027s in the context.\n\nIn the API, the RequestSpec.project_id and user_id get set by this call:\n\nhttps://github.com/openstack/nova/blob/9c024e45bb6bd66a73e6cce8b739455799ff564c/nova/compute/api.py#L886\n\nhttps://github.com/openstack/nova/blob/9c024e45bb6bd66a73e6cce8b739455799ff564c/nova/objects/request_spec.py#L463-L464\n\nTherefore we can definitely set the InstanceMapping.user_id here as well:\n\nhttps://github.com/openstack/nova/blob/9c024e45bb6bd66a73e6cce8b739455799ff564c/nova/compute/api.py#L949","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"974ffcb08f7f85e2c2f12a7d91d55c58a7da149f","unresolved":false,"context_lines":[{"line_number":15,"context_line":"fact that already existing instance mappings, that have not yet"},{"line_number":16,"context_line":"scheduled the instance to a cell, will be unable to be migrated to"},{"line_number":17,"context_line":"populate user_id until they are scheduled. We need the"},{"line_number":18,"context_line":"instance.user_id from the cell to know what user_id to set."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"We expect callers of InstanceMapping.create to have specified user_id"},{"line_number":21,"context_line":"when creating an instance mapping, but if they do not, we default"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9fdfeff1_53a50b02","line":18,"in_reply_to":"9fdfeff1_987ab085","updated":"2019-02-27 00:29:13.000000000","message":"Well, I hastily began making the change I described earlier, without first checking whether build request has a user_id field. Turns out it doesn\u0027t. :(\n\nOther than that, we have request spec, but AFAICT in the code, project_id and user_id aren\u0027t set on it until the request gets to conductor. So, there\u0027s a window of time where request spec exists (in the API and part of conductor) and has a NULL user_id. Because of this, I think we can\u0027t depend on it 100% for populating InstanceMapping.user_id.\n\nIt seems to me, no matter what, we can\u0027t be 100% able to get user_id at just any point in time (there will be small windows where it\u0027s not possible to get it from anywhere). Based on that, I think we have to stay with what we have currently here. Let me know if I\u0027m wrong on that and I\u0027d be happy to make changes to make user_id non-nullable, if there\u0027s a way to do it.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"bc2e905ee2f4a2f33e8d70baf4dfae40edaf814a","unresolved":false,"context_lines":[{"line_number":15,"context_line":"fact that already existing instance mappings, that have not yet"},{"line_number":16,"context_line":"scheduled the instance to a cell, will be unable to be migrated to"},{"line_number":17,"context_line":"populate user_id until they are scheduled. We need the"},{"line_number":18,"context_line":"instance.user_id from the cell to know what user_id to set."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"We expect callers of InstanceMapping.create to have specified user_id"},{"line_number":21,"context_line":"when creating an instance mapping, but if they do not, we default"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"9fdfeff1_987ab085","line":18,"in_reply_to":"9fdfeff1_c4429b3b","updated":"2019-02-26 22:56:43.000000000","message":"Build requests are ephemeral and only live until the point at which the instance becomes scheduled. So hm.. it would seem I could use build request if cell_mapping\u003dNone. At that point, I think the only concern would be a race where I get cell_mapping\u003dNone but BuildRequestNotFound, but then if I do, I could maybe refresh cell_mapping and do an instance lookup. So maybe that would work fine.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"}],"nova/compute/api.py":[{"author":{"_account_id":26936,"name":"Surya Seetharaman","email":"suryaseetharaman.9@gmail.com","username":"tssurya"},"change_message_id":"25fa94a631267dc5efec12fe0e5192bcac20b79b","unresolved":false,"context_lines":[{"line_number":2186,"context_line":"            instance.destroy()"},{"line_number":2187,"context_line":""},{"line_number":2188,"context_line":"    @staticmethod"},{"line_number":2189,"context_line":"    def _update_queued_for_deletion_and_user_id(context, instance, qfd):"},{"line_number":2190,"context_line":"        # NOTE(tssurya): We query the instance_mapping record of this instance"},{"line_number":2191,"context_line":"        # and update the queued_for_delete flag to True (or False according to"},{"line_number":2192,"context_line":"        # the state of the instance). This just means that the instance is"}],"source_content_type":"text/x-python","patch_set":6,"id":"9fdfeff1_f3d8231f","line":2189,"range":{"start_line":2189,"start_character":8,"end_line":2189,"end_character":47},"updated":"2019-02-21 13:16:32.000000000","message":"Why are we changing this portion of code to update user_id also when an instance gets deleted ? Wouldn\u0027t the migration take care of all the existing mappings? And you are updating the user_id on L950 before saving the mapping for those scheduling case. Not sure why we need to handle the delete scenarios","commit_id":"a59198ae4940f27551166cbaeddc8ceedb20ff24"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a38a141c7d7eddd86b096478550cfd9ec8b217aa","unresolved":false,"context_lines":[{"line_number":2186,"context_line":"            instance.destroy()"},{"line_number":2187,"context_line":""},{"line_number":2188,"context_line":"    @staticmethod"},{"line_number":2189,"context_line":"    def _update_queued_for_deletion_and_user_id(context, instance, qfd):"},{"line_number":2190,"context_line":"        # NOTE(tssurya): We query the instance_mapping record of this instance"},{"line_number":2191,"context_line":"        # and update the queued_for_delete flag to True (or False according to"},{"line_number":2192,"context_line":"        # the state of the instance). This just means that the instance is"}],"source_content_type":"text/x-python","patch_set":6,"id":"9fdfeff1_79de4d11","line":2189,"range":{"start_line":2189,"start_character":8,"end_line":2189,"end_character":47},"in_reply_to":"9fdfeff1_7985ed58","updated":"2019-02-21 17:25:25.000000000","message":"Oh, I remember now why I did this, I was thinking about not querying the instance mapping from the database twice in the case of \u0027restore\u0027 and to do it in one batch.","commit_id":"a59198ae4940f27551166cbaeddc8ceedb20ff24"},{"author":{"_account_id":26936,"name":"Surya Seetharaman","email":"suryaseetharaman.9@gmail.com","username":"tssurya"},"change_message_id":"b022d426821cf0489bf3724758b554e535d97f5e","unresolved":false,"context_lines":[{"line_number":2186,"context_line":"            instance.destroy()"},{"line_number":2187,"context_line":""},{"line_number":2188,"context_line":"    @staticmethod"},{"line_number":2189,"context_line":"    def _update_queued_for_deletion_and_user_id(context, instance, qfd):"},{"line_number":2190,"context_line":"        # NOTE(tssurya): We query the instance_mapping record of this instance"},{"line_number":2191,"context_line":"        # and update the queued_for_delete flag to True (or False according to"},{"line_number":2192,"context_line":"        # the state of the instance). This just means that the instance is"}],"source_content_type":"text/x-python","patch_set":6,"id":"9fdfeff1_82f13a6d","line":2189,"range":{"start_line":2189,"start_character":8,"end_line":2189,"end_character":47},"in_reply_to":"9fdfeff1_79de4d11","updated":"2019-02-21 20:16:42.000000000","message":"oh! okay, so for soft delete case (since in the data migration we have decided to not bother updating deleting instances,) you need to update if they come back from the dead, hence you are doing this here except that this became a common platform for all the deletes. Got it!","commit_id":"a59198ae4940f27551166cbaeddc8ceedb20ff24"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"29d0904bd913384cc71a37e5fd43903031e883df","unresolved":false,"context_lines":[{"line_number":2186,"context_line":"            instance.destroy()"},{"line_number":2187,"context_line":""},{"line_number":2188,"context_line":"    @staticmethod"},{"line_number":2189,"context_line":"    def _update_queued_for_deletion_and_user_id(context, instance, qfd):"},{"line_number":2190,"context_line":"        # NOTE(tssurya): We query the instance_mapping record of this instance"},{"line_number":2191,"context_line":"        # and update the queued_for_delete flag to True (or False according to"},{"line_number":2192,"context_line":"        # the state of the instance). This just means that the instance is"}],"source_content_type":"text/x-python","patch_set":6,"id":"9fdfeff1_7985ed58","line":2189,"range":{"start_line":2189,"start_character":8,"end_line":2189,"end_character":47},"in_reply_to":"9fdfeff1_f3d8231f","updated":"2019-02-21 17:12:11.000000000","message":"Oh, hm. I didn\u0027t think about updating this when an instance gets deleted and intended to update user_id (if it is None) during a \u0027restore\u0027 of a soft-deleted instance. So I think you are right, I shouldn\u0027t do this here, I should do it only specifically for \u0027restore\u0027. Thanks for calling this out.","commit_id":"a59198ae4940f27551166cbaeddc8ceedb20ff24"},{"author":{"_account_id":26936,"name":"Surya Seetharaman","email":"suryaseetharaman.9@gmail.com","username":"tssurya"},"change_message_id":"b022d426821cf0489bf3724758b554e535d97f5e","unresolved":false,"context_lines":[{"line_number":2285,"context_line":"            instance.task_state \u003d None"},{"line_number":2286,"context_line":"            instance.deleted_at \u003d None"},{"line_number":2287,"context_line":"            instance.save(expected_task_state\u003d[None])"},{"line_number":2288,"context_line":"        self._update_queued_for_deletion_and_user_id(context, instance, False)"},{"line_number":2289,"context_line":""},{"line_number":2290,"context_line":"    @check_instance_lock"},{"line_number":2291,"context_line":"    @check_instance_state(task_state\u003dNone,"}],"source_content_type":"text/x-python","patch_set":6,"id":"9fdfeff1_42b91277","line":2288,"range":{"start_line":2288,"start_character":41,"end_line":2288,"end_character":78},"updated":"2019-02-21 20:16:42.000000000","message":"so its at this part basically if they get restored we need to go set their user_id if its None in the mapping. makes sense.\n\nSince we need not do this for other kinds of delete, maybe we can pass user_id\u003dNone in the _update_queued_for_deletion_and_user_id() function ? and only set it for this case or something ? Just felt that commonly doing this for all deletes doesn\u0027t make sense but avoiding double querying seperately does make sense","commit_id":"a59198ae4940f27551166cbaeddc8ceedb20ff24"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a28ebeb61deaed732845a0908071784226b11430","unresolved":false,"context_lines":[{"line_number":2285,"context_line":"            instance.task_state \u003d None"},{"line_number":2286,"context_line":"            instance.deleted_at \u003d None"},{"line_number":2287,"context_line":"            instance.save(expected_task_state\u003d[None])"},{"line_number":2288,"context_line":"        self._update_queued_for_deletion_and_user_id(context, instance, False)"},{"line_number":2289,"context_line":""},{"line_number":2290,"context_line":"    @check_instance_lock"},{"line_number":2291,"context_line":"    @check_instance_state(task_state\u003dNone,"}],"source_content_type":"text/x-python","patch_set":6,"id":"9fdfeff1_edd59748","line":2288,"range":{"start_line":2288,"start_character":41,"end_line":2288,"end_character":78},"in_reply_to":"9fdfeff1_42b91277","updated":"2019-02-21 20:30:58.000000000","message":"Yeah, sorry I didn\u0027t mention it earlier but I intended to try to find another way about it (I thought you\u0027d be off by now! :) ). Currently, I\u0027ve got a local change to return the InstanceMapping object from _update_queued_for_deletion and just set user_id and save it again in the restore method. It\u0027s still an extra save, but I think it\u0027s not nearly so bad as reading the row twice?","commit_id":"a59198ae4940f27551166cbaeddc8ceedb20ff24"},{"author":{"_account_id":26936,"name":"Surya Seetharaman","email":"suryaseetharaman.9@gmail.com","username":"tssurya"},"change_message_id":"9fb0fbaf6ed69e8e9b35ce47a4c70525141e02b0","unresolved":false,"context_lines":[{"line_number":2285,"context_line":"            instance.task_state \u003d None"},{"line_number":2286,"context_line":"            instance.deleted_at \u003d None"},{"line_number":2287,"context_line":"            instance.save(expected_task_state\u003d[None])"},{"line_number":2288,"context_line":"        self._update_queued_for_deletion_and_user_id(context, instance, False)"},{"line_number":2289,"context_line":""},{"line_number":2290,"context_line":"    @check_instance_lock"},{"line_number":2291,"context_line":"    @check_instance_state(task_state\u003dNone,"}],"source_content_type":"text/x-python","patch_set":6,"id":"9fdfeff1_4c713ca8","line":2288,"range":{"start_line":2288,"start_character":41,"end_line":2288,"end_character":78},"in_reply_to":"9fdfeff1_edd59748","updated":"2019-02-22 08:41:16.000000000","message":"Let\u0027s continue this in the patchset: https://review.openstack.org/#/c/638574/1/nova/compute/api.py@2288 where I have described what I meant earlier in my previous comment :D .","commit_id":"a59198ae4940f27551166cbaeddc8ceedb20ff24"}],"nova/objects/instance_mapping.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"de6f7cb939f63a26e83881b48f91481a732e4366","unresolved":false,"context_lines":[{"line_number":39,"context_line":"        \u0027project_id\u0027: fields.StringField(),"},{"line_number":40,"context_line":"        # TODO(melwitt): user_id should be changed to non-nullable at the next"},{"line_number":41,"context_line":"        # major version bump."},{"line_number":42,"context_line":"        \u0027user_id\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":43,"context_line":"        \u0027queued_for_delete\u0027: fields.BooleanField(default\u003dFalse),"},{"line_number":44,"context_line":"        }"},{"line_number":45,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_8626bcca","line":42,"updated":"2019-02-27 19:46:56.000000000","message":"I don\u0027t think this should be nullable. I think if we load this from the DB and the column there is null, we should look further to find out what it is (i.e. look at the reqspec, or buildreq if it\u0027s there, or maybe even look in the cell).\n\nI\u0027m not sure where this is used yet, but since it wouldn\u0027t be for quota-querying purposes anyway, I don\u0027t think there\u0027s any good reason to have this be nullable for compatibility while we\u0027re migrating.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dca490d05e1128ca2f4f8d2f4382672de2d08ab5","unresolved":false,"context_lines":[{"line_number":39,"context_line":"        \u0027project_id\u0027: fields.StringField(),"},{"line_number":40,"context_line":"        # TODO(melwitt): user_id should be changed to non-nullable at the next"},{"line_number":41,"context_line":"        # major version bump."},{"line_number":42,"context_line":"        \u0027user_id\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":43,"context_line":"        \u0027queued_for_delete\u0027: fields.BooleanField(default\u003dFalse),"},{"line_number":44,"context_line":"        }"},{"line_number":45,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_cf201db5","line":42,"in_reply_to":"9fdfeff1_1670b5a3","updated":"2019-03-01 20:20:24.000000000","message":"\u003e I will leave this as nullable because request spec only stores user_id as a json blob\n\nI\u0027m not sure what that has to do with this, unless you were going to filter on the RequestSpec.user_id somehow in a DB query on load?\n\n\u003e  build request doesn\u0027t have a user_id field.\n\nNot true, BuildRequest has an instance field which is itself a json blob and within that there is the user_id.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"bb3a0dbf42ae21b69bc3a5697e93d758d8075f65","unresolved":false,"context_lines":[{"line_number":39,"context_line":"        \u0027project_id\u0027: fields.StringField(),"},{"line_number":40,"context_line":"        # TODO(melwitt): user_id should be changed to non-nullable at the next"},{"line_number":41,"context_line":"        # major version bump."},{"line_number":42,"context_line":"        \u0027user_id\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":43,"context_line":"        \u0027queued_for_delete\u0027: fields.BooleanField(default\u003dFalse),"},{"line_number":44,"context_line":"        }"},{"line_number":45,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_a3e2922f","line":42,"in_reply_to":"9fdfeff1_8626bcca","updated":"2019-03-01 22:50:16.000000000","message":"\u003e I don\u0027t think this should be nullable. I think if we load this from\n \u003e the DB and the column there is null, we should look further to find\n \u003e out what it is (i.e. look at the reqspec, or buildreq if it\u0027s\n \u003e there, or maybe even look in the cell).\n \u003e \n \u003e I\u0027m not sure where this is used yet, but since it wouldn\u0027t be for\n \u003e quota-querying purposes anyway, I don\u0027t think there\u0027s any good\n \u003e reason to have this be nullable for compatibility while we\u0027re\n \u003e migrating.\n\nTBC, the InstanceMapping.user_id field will be used for quota-querying purposes (quota usage counting of instances).","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d46e63e5c492b5b8d14b66baaee7890ffc0d4ded","unresolved":false,"context_lines":[{"line_number":39,"context_line":"        \u0027project_id\u0027: fields.StringField(),"},{"line_number":40,"context_line":"        # TODO(melwitt): user_id should be changed to non-nullable at the next"},{"line_number":41,"context_line":"        # major version bump."},{"line_number":42,"context_line":"        \u0027user_id\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":43,"context_line":"        \u0027queued_for_delete\u0027: fields.BooleanField(default\u003dFalse),"},{"line_number":44,"context_line":"        }"},{"line_number":45,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_8c5b3b20","line":42,"in_reply_to":"9fdfeff1_8626bcca","updated":"2019-02-27 20:31:25.000000000","message":"\u003e I don\u0027t think this should be nullable. I think if we load this from the DB and the column there is null, we should look further to find out what it is (i.e. look at the reqspec, or buildreq if it\u0027s there, or maybe even look in the cell).\n\nDo you mean that in _from_db_object if db_instance_mapping[\u0027user_id\u0027] is None, you want to lookup the RequestSpec to get the user_id, and if it\u0027s not set there (because it\u0027s old), lookup the BuildRequest and if that is not found lookup the instance in a cell (if cell_mapping is set)?\n\nThat seems pretty heavy. I mean, it could work, and it would be an opportunistic data migration, but still seems heavy - although it\u0027s a one-time cost I guess.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"230ee439122b3021ada5cb1062dd34daa7b27ef9","unresolved":false,"context_lines":[{"line_number":39,"context_line":"        \u0027project_id\u0027: fields.StringField(),"},{"line_number":40,"context_line":"        # TODO(melwitt): user_id should be changed to non-nullable at the next"},{"line_number":41,"context_line":"        # major version bump."},{"line_number":42,"context_line":"        \u0027user_id\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":43,"context_line":"        \u0027queued_for_delete\u0027: fields.BooleanField(default\u003dFalse),"},{"line_number":44,"context_line":"        }"},{"line_number":45,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_1670b5a3","line":42,"in_reply_to":"9fdfeff1_8c5b3b20","updated":"2019-02-27 22:41:22.000000000","message":"IIUC, based on our IRC discussion today, I will leave this as nullable because request spec only stores user_id as a json blob and build request doesn\u0027t have a user_id field.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"bf7a03e0286728e4432334264a6bae18a891b6d4","unresolved":false,"context_lines":[{"line_number":39,"context_line":"        \u0027project_id\u0027: fields.StringField(),"},{"line_number":40,"context_line":"        # TODO(melwitt): user_id should be changed to non-nullable at the next"},{"line_number":41,"context_line":"        # major version bump."},{"line_number":42,"context_line":"        \u0027user_id\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":43,"context_line":"        \u0027queued_for_delete\u0027: fields.BooleanField(default\u003dFalse),"},{"line_number":44,"context_line":"        }"},{"line_number":45,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"5fc1f717_f67296e3","line":42,"in_reply_to":"9fdfeff1_cf201db5","updated":"2019-03-05 20:08:52.000000000","message":"\u003e \u003e  build request doesn\u0027t have a user_id field.\n \u003e \n \u003e Not true, BuildRequest has an instance field which is itself a json\n \u003e blob and within that there is the user_id.\n\nI think \"build request doesn\u0027t have a user_id field\" was indeed a true statement, as you alluded to in your comment here. Yes, there\u0027s an instance field JSON blob that has a user_id in it, but that\u0027s not the same as a user_id field on BuildRequest.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"de6f7cb939f63a26e83881b48f91481a732e4366","unresolved":false,"context_lines":[{"line_number":120,"context_line":"            # contained in our RequestContext. The user_id field is intended"},{"line_number":121,"context_line":"            # to be non-nullable but we must allow NULL while we migrate"},{"line_number":122,"context_line":"            # already existing instance mappings."},{"line_number":123,"context_line":"            changes[\u0027user_id\u0027] \u003d self._context.user_id"},{"line_number":124,"context_line":"        db_mapping \u003d self._create_in_db(self._context, changes)"},{"line_number":125,"context_line":"        self._from_db_object(self._context, self, db_mapping)"},{"line_number":126,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_86ff9c00","line":123,"updated":"2019-02-27 19:46:56.000000000","message":"This seems to me to just be papering over the fact that callers to create() need to be providing this value now. Since I expect instance mappings are created infrequently in the code (just in one place in compute/api right?) this seems like it\u0027s just there to avoid having to change a bunch of unit tests, which is not a good reason, IMHO.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dca490d05e1128ca2f4f8d2f4382672de2d08ab5","unresolved":false,"context_lines":[{"line_number":120,"context_line":"            # contained in our RequestContext. The user_id field is intended"},{"line_number":121,"context_line":"            # to be non-nullable but we must allow NULL while we migrate"},{"line_number":122,"context_line":"            # already existing instance mappings."},{"line_number":123,"context_line":"            changes[\u0027user_id\u0027] \u003d self._context.user_id"},{"line_number":124,"context_line":"        db_mapping \u003d self._create_in_db(self._context, changes)"},{"line_number":125,"context_line":"        self._from_db_object(self._context, self, db_mapping)"},{"line_number":126,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_f20b861f","line":123,"in_reply_to":"9fdfeff1_3db7cd23","updated":"2019-03-01 20:20:24.000000000","message":"\u003e Yea with all the places where we are creating/updating the\n \u003e instance_mapping I was thinking of a scenario where its just NULL\n \u003e and then the migration goes on forever. But if you are confident we\n \u003e have handles setting it every time we create a new instance_mapping\n \u003e I guess we can remove it.\n \u003e \n \u003e @melwitt: Sorry if it seems redundant at this point.\n\nThe only place an InstanceMapping is created in runtime code is in compute API _provision_instances. If you\u0027re referring to online data migrations, I think we squashed that one place.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":26936,"name":"Surya Seetharaman","email":"suryaseetharaman.9@gmail.com","username":"tssurya"},"change_message_id":"b616b95c77518c58fc4cd517d7dc52db742104c9","unresolved":false,"context_lines":[{"line_number":120,"context_line":"            # contained in our RequestContext. The user_id field is intended"},{"line_number":121,"context_line":"            # to be non-nullable but we must allow NULL while we migrate"},{"line_number":122,"context_line":"            # already existing instance mappings."},{"line_number":123,"context_line":"            changes[\u0027user_id\u0027] \u003d self._context.user_id"},{"line_number":124,"context_line":"        db_mapping \u003d self._create_in_db(self._context, changes)"},{"line_number":125,"context_line":"        self._from_db_object(self._context, self, db_mapping)"},{"line_number":126,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_3db7cd23","line":123,"in_reply_to":"9fdfeff1_569f1db3","updated":"2019-02-28 12:15:38.000000000","message":"Yea with all the places where we are creating/updating the instance_mapping I was thinking of a scenario where its just NULL and then the migration goes on forever. But if you are confident we have handles setting it every time we create a new instance_mapping I guess we can remove it. \n\n@melwitt: Sorry if it seems redundant at this point.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d46e63e5c492b5b8d14b66baaee7890ffc0d4ded","unresolved":false,"context_lines":[{"line_number":120,"context_line":"            # contained in our RequestContext. The user_id field is intended"},{"line_number":121,"context_line":"            # to be non-nullable but we must allow NULL while we migrate"},{"line_number":122,"context_line":"            # already existing instance mappings."},{"line_number":123,"context_line":"            changes[\u0027user_id\u0027] \u003d self._context.user_id"},{"line_number":124,"context_line":"        db_mapping \u003d self._create_in_db(self._context, changes)"},{"line_number":125,"context_line":"        self._from_db_object(self._context, self, db_mapping)"},{"line_number":126,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_8c89fb6f","line":123,"in_reply_to":"9fdfeff1_86ff9c00","updated":"2019-02-27 20:31:25.000000000","message":"Agree that I don\u0027t think we need this unless like queued_for_delete above we need it missing for testing, but I\u0027d think for testing with a null user_id we could do that by putting something in the database first and then pull it out with the InstanceMapping object.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"230ee439122b3021ada5cb1062dd34daa7b27ef9","unresolved":false,"context_lines":[{"line_number":120,"context_line":"            # contained in our RequestContext. The user_id field is intended"},{"line_number":121,"context_line":"            # to be non-nullable but we must allow NULL while we migrate"},{"line_number":122,"context_line":"            # already existing instance mappings."},{"line_number":123,"context_line":"            changes[\u0027user_id\u0027] \u003d self._context.user_id"},{"line_number":124,"context_line":"        db_mapping \u003d self._create_in_db(self._context, changes)"},{"line_number":125,"context_line":"        self._from_db_object(self._context, self, db_mapping)"},{"line_number":126,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_569f1db3","line":123,"in_reply_to":"9fdfeff1_8c89fb6f","updated":"2019-02-27 22:41:22.000000000","message":"I added it here after Surya pointed out, in the context of grenade going into an infinite loop on an earlier PS, the potential for an infinite migration if someone for some reason did a InstanceMapping.create somewhere in the code and never set user_id for it (endless loop of user_id-less instance mappings being created and migrated). I can remove it if you think it\u0027s not useful.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"60ea3b219ecb8a2ca7ab45be2fc848ea06219e5c","unresolved":false,"context_lines":[{"line_number":211,"context_line":"    # Version 1.0: Initial version"},{"line_number":212,"context_line":"    # Version 1.1: Added get_by_cell_id method."},{"line_number":213,"context_line":"    # Version 1.2: Added get_by_instance_uuids method"},{"line_number":214,"context_line":"    # Version 1.3: InstanceMapping \u003c\u003d version 1.2"},{"line_number":215,"context_line":"    VERSION \u003d \u00271.3\u0027"},{"line_number":216,"context_line":""},{"line_number":217,"context_line":"    fields \u003d {"},{"line_number":218,"context_line":"        \u0027objects\u0027: fields.ListOfObjectsField(\u0027InstanceMapping\u0027),"}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_0b46557c","line":215,"range":{"start_line":214,"start_character":4,"end_line":215,"end_character":19},"updated":"2019-02-27 19:07:10.000000000","message":"Huh? We haven\u0027t had to do this for years. The manifest takes care of this for us.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"de6f7cb939f63a26e83881b48f91481a732e4366","unresolved":false,"context_lines":[{"line_number":211,"context_line":"    # Version 1.0: Initial version"},{"line_number":212,"context_line":"    # Version 1.1: Added get_by_cell_id method."},{"line_number":213,"context_line":"    # Version 1.2: Added get_by_instance_uuids method"},{"line_number":214,"context_line":"    # Version 1.3: InstanceMapping \u003c\u003d version 1.2"},{"line_number":215,"context_line":"    VERSION \u003d \u00271.3\u0027"},{"line_number":216,"context_line":""},{"line_number":217,"context_line":"    fields \u003d {"},{"line_number":218,"context_line":"        \u0027objects\u0027: fields.ListOfObjectsField(\u0027InstanceMapping\u0027),"}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_1109a429","line":215,"range":{"start_line":214,"start_character":4,"end_line":215,"end_character":19},"in_reply_to":"9fdfeff1_0b46557c","updated":"2019-02-27 19:46:56.000000000","message":"Correct, this is not necessary.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"7273ab4598fa8a58b3871d4c3a5549f2e329cca2","unresolved":false,"context_lines":[{"line_number":211,"context_line":"    # Version 1.0: Initial version"},{"line_number":212,"context_line":"    # Version 1.1: Added get_by_cell_id method."},{"line_number":213,"context_line":"    # Version 1.2: Added get_by_instance_uuids method"},{"line_number":214,"context_line":"    # Version 1.3: InstanceMapping \u003c\u003d version 1.2"},{"line_number":215,"context_line":"    VERSION \u003d \u00271.3\u0027"},{"line_number":216,"context_line":""},{"line_number":217,"context_line":"    fields \u003d {"},{"line_number":218,"context_line":"        \u0027objects\u0027: fields.ListOfObjectsField(\u0027InstanceMapping\u0027),"}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_06eacc72","line":215,"range":{"start_line":214,"start_character":4,"end_line":215,"end_character":19},"in_reply_to":"9fdfeff1_0b46557c","updated":"2019-02-27 19:50:13.000000000","message":"That goes to show you when was the last time I added a field to an object.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c303354dd491e093de3f4afa958e21800a966db9","unresolved":false,"context_lines":[{"line_number":39,"context_line":"        \u0027project_id\u0027: fields.StringField(),"},{"line_number":40,"context_line":"        # TODO(melwitt): user_id should be changed to non-nullable at the next"},{"line_number":41,"context_line":"        # major version bump."},{"line_number":42,"context_line":"        \u0027user_id\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":43,"context_line":"        \u0027queued_for_delete\u0027: fields.BooleanField(default\u003dFalse),"},{"line_number":44,"context_line":"        }"},{"line_number":45,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"9fdfeff1_d26f6291","line":42,"range":{"start_line":42,"start_character":38,"end_line":42,"end_character":51},"updated":"2019-03-01 20:35:04.000000000","message":"I think this might be unresolved from https://review.openstack.org/#/c/633350/7/nova/objects/instance_mapping.py@42.\n\nTechnically we can try to get the user_id from the RequestSpec *and* the BuildRequest:\n\n- RequestSpec.user_id\n- BuildRequest.instance.user_id\n\nFailing those, if cell_mapping is set, we can try to get the Instance record via the cell. But if the cell is down, then we\u0027re pretty much screwed, right? What would we do, raise InstanceMappingNotFound or something? Otherwise it could result in a 500 response when trying to do GET /servers/{server_id}, but maybe that\u0027s OK because it is a 500 kind of situation, but you probably can\u0027t get out of it until the cell is back up since the online data migration is going to rely on talking to the cell as well to get the Instance.user_id.","commit_id":"8de9b00589f32359ee56b85fefce002234ac833a"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"63a991c9202b8aeaff50748387092868e78609b3","unresolved":false,"context_lines":[{"line_number":39,"context_line":"        \u0027project_id\u0027: fields.StringField(),"},{"line_number":40,"context_line":"        # TODO(melwitt): user_id should be changed to non-nullable at the next"},{"line_number":41,"context_line":"        # major version bump."},{"line_number":42,"context_line":"        \u0027user_id\u0027: fields.StringField(nullable\u003dTrue),"},{"line_number":43,"context_line":"        \u0027queued_for_delete\u0027: fields.BooleanField(default\u003dFalse),"},{"line_number":44,"context_line":"        }"},{"line_number":45,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"9fdfeff1_632a6a30","line":42,"range":{"start_line":42,"start_character":38,"end_line":42,"end_character":51},"in_reply_to":"9fdfeff1_d26f6291","updated":"2019-03-01 23:01:18.000000000","message":"Apologies, as I mentioned in IRC today, I didn\u0027t understand that the request to make the field non-nullable still stood. Last time we chatted in IRC, Dan had suggested not adding the user_id column at all, and use request spec instead. Once we learned both request spec and build request stored instance user_id as json blobs, we knew we needed the column after all.\n\nThat made me think the patch was OK as-is and I missed the part where the request to make the field non-nullable still stood. So, I can try and add the above try/fallback approach you describe. And I guess, yeah, we would be looking at failing with a 500 if we can\u0027t populate the user_id from anywhere.","commit_id":"8de9b00589f32359ee56b85fefce002234ac833a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9a7d13db69e0891453ccf90da4cf341e54c5c2ba","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            pass"},{"line_number":87,"context_line":"        if user_id is not None:"},{"line_number":88,"context_line":"            return user_id"},{"line_number":89,"context_line":"        # Finally, try to get the instance from the cell (if it has a cell)."},{"line_number":90,"context_line":"        if cell is not None:"},{"line_number":91,"context_line":"            try:"},{"line_number":92,"context_line":"                with nova_context.target_cell(context, cell) as cctxt:"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_8085e0b0","line":89,"updated":"2019-03-04 22:46:44.000000000","message":"Hmm, why try this last? The cell is going to be the fastest option right? Especially if you add a Instance.get_user_id(instance_uuid) call, you\u0027ll query out just that one thing and avoid the expensive JSON loads of the above. If the cell is down/disabled, then falling back to the JSON-based structures above is maybe worth it.\n\nFrom skimming the patches above, I\u0027m guessing you\u0027re just going to end up doing this for every instance sequentially which is going to be a really ugly performance hit. You\u0027d gain a lot by being able to batch a bunch of these, but I guess it\u0027s just a hit until the migrations are run.","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"c3e55eb5715b25e14f1ae25147802dc8f16224e0","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            pass"},{"line_number":87,"context_line":"        if user_id is not None:"},{"line_number":88,"context_line":"            return user_id"},{"line_number":89,"context_line":"        # Finally, try to get the instance from the cell (if it has a cell)."},{"line_number":90,"context_line":"        if cell is not None:"},{"line_number":91,"context_line":"            try:"},{"line_number":92,"context_line":"                with nova_context.target_cell(context, cell) as cctxt:"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_5491f952","line":89,"in_reply_to":"9fdfeff1_5447b91f","updated":"2019-03-04 23:16:43.000000000","message":"To be clear, when we scatter_gather_single_cell (to get down cell handling), a down cell will mean we time out waiting (60 seconds) before we\u0027d fall back to request spec or build request.","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":15888,"name":"Zhenyu Zheng","email":"zheng.zhenyu@outlook.com","username":"Kevin_Zheng"},"change_message_id":"159a05ce8f83d0ba18a89fd77c352cf93988d804","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            pass"},{"line_number":87,"context_line":"        if user_id is not None:"},{"line_number":88,"context_line":"            return user_id"},{"line_number":89,"context_line":"        # Finally, try to get the instance from the cell (if it has a cell)."},{"line_number":90,"context_line":"        if cell is not None:"},{"line_number":91,"context_line":"            try:"},{"line_number":92,"context_line":"                with nova_context.target_cell(context, cell) as cctxt:"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_b7e867b3","line":89,"in_reply_to":"9fdfeff1_5491f952","updated":"2019-03-05 00:28:52.000000000","message":"could we make CELL_TIMEOUT a parameter that can be passed and default to 60? so in this case we can pass some shorter timeout","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"80e28e0121bad1d762321cb3735117249fd18808","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            pass"},{"line_number":87,"context_line":"        if user_id is not None:"},{"line_number":88,"context_line":"            return user_id"},{"line_number":89,"context_line":"        # Finally, try to get the instance from the cell (if it has a cell)."},{"line_number":90,"context_line":"        if cell is not None:"},{"line_number":91,"context_line":"            try:"},{"line_number":92,"context_line":"                with nova_context.target_cell(context, cell) as cctxt:"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_5447b91f","line":89,"in_reply_to":"9fdfeff1_8085e0b0","updated":"2019-03-04 23:12:07.000000000","message":"I put the get instance from cell attempt last since it depends on cells being up, but I think what you say makes sense about it being the fastest option and best to try first and fall back on other options if a cell is down.\n\nAlthough, I think we\u0027d need to use the scatter_gather_single_cell for this (even needed for what I have here) and we\u0027d get a Timeout exception if the cell is down. I think maybe that\u0027s why I was feeling a hunch that getting from the cell could be a big perf problem if a cell is down.","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":15888,"name":"Zhenyu Zheng","email":"zheng.zhenyu@outlook.com","username":"Kevin_Zheng"},"change_message_id":"81c439140a31374d8df97dc8acf68ef64041c797","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            pass"},{"line_number":87,"context_line":"        if user_id is not None:"},{"line_number":88,"context_line":"            return user_id"},{"line_number":89,"context_line":"        # Finally, try to get the instance from the cell (if it has a cell)."},{"line_number":90,"context_line":"        if cell is not None:"},{"line_number":91,"context_line":"            try:"},{"line_number":92,"context_line":"                with nova_context.target_cell(context, cell) as cctxt:"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_f7634f40","line":89,"in_reply_to":"9fdfeff1_b7e867b3","updated":"2019-03-05 00:35:35.000000000","message":"The current code seems also valid? why couldn\u0027t we just use it?","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f30a3bcb93f6a03188acda30f965a94fc9efbf34","unresolved":false,"context_lines":[{"line_number":86,"context_line":"            pass"},{"line_number":87,"context_line":"        if user_id is not None:"},{"line_number":88,"context_line":"            return user_id"},{"line_number":89,"context_line":"        # Finally, try to get the instance from the cell (if it has a cell)."},{"line_number":90,"context_line":"        if cell is not None:"},{"line_number":91,"context_line":"            try:"},{"line_number":92,"context_line":"                with nova_context.target_cell(context, cell) as cctxt:"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_ea77e69e","line":89,"in_reply_to":"9fdfeff1_f7634f40","updated":"2019-03-05 01:00:35.000000000","message":"We could use it. BUT, I think Dan\u0027s point is that none of the patches stacked on top of this one _read_ the user_id field. And if no one is ultimately reading the user_id field, we don\u0027t really need to go to the trouble of populating it during _from_db_object. We could instead leave it unset if it\u0027s NULL in the database.\n\nAll of the quota counting code later in the series reads raw database records and does not make use of the user_id field. The only way I\u0027m using the user_id field is to write it, when we InstanceMapping.create the instance mapping record in compute/api and a few other places (nova-manage virtual interface migration and map_instances routine).\n\nSo, it seems I did all of this _from_db_object populate for no usefulness. :( I would prefer to be able to delete this code trying to populate user_id and just leave user_id unset if it\u0027s found to be NULL. Reason being, this code is messy and introduces a performance hit, at the same time that it\u0027s not needed by any code later in the series.\n\nI had kept thinking \"user_id is non-nullable, it must be set at all times\" and did not realize leaving it unset would be a reasonable thing to do, if there are no known intended readers of the user_id field.","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9a7d13db69e0891453ccf90da4cf341e54c5c2ba","unresolved":false,"context_lines":[{"line_number":127,"context_line":"                instance_mapping.user_id \u003d user_id"},{"line_number":128,"context_line":"            else:"},{"line_number":129,"context_line":"                raise exception.InstanceMappingNotFound("},{"line_number":130,"context_line":"                    uuid\u003dinstance_mapping.instance_uuid)"},{"line_number":131,"context_line":"        instance_mapping.obj_reset_changes()"},{"line_number":132,"context_line":"        instance_mapping._context \u003d context"},{"line_number":133,"context_line":"        return instance_mapping"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_2033d415","line":130,"updated":"2019-03-04 22:46:44.000000000","message":"This is a really odd thing to raise out of _from_db_object(). We\u0027ve basically said \"here, I found this in the database, convert it to an object\" only to have this explode and say \"that thing doesn\u0027t exist\". Right?\n\nSo, I agree that this needs to explode, but this is probably not the best exception to use. Presumably whatever this is has to be handled in the thing that does the bulk query of mappings...is that get_counts()?","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"5aff4dc7cfc0145e7c69be45e6d857a6aa4fdeb1","unresolved":false,"context_lines":[{"line_number":127,"context_line":"                instance_mapping.user_id \u003d user_id"},{"line_number":128,"context_line":"            else:"},{"line_number":129,"context_line":"                raise exception.InstanceMappingNotFound("},{"line_number":130,"context_line":"                    uuid\u003dinstance_mapping.instance_uuid)"},{"line_number":131,"context_line":"        instance_mapping.obj_reset_changes()"},{"line_number":132,"context_line":"        instance_mapping._context \u003d context"},{"line_number":133,"context_line":"        return instance_mapping"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_724520b4","line":130,"in_reply_to":"9fdfeff1_14edb1ef","updated":"2019-03-05 14:43:12.000000000","message":"I think you should add it to the object, but you\u0027re jumping through a lot of hoops to make it loadable when we\u0027re not actually ever going to use it (currently) on the object as read from the DB. So, yeah, why not just not load it here if it\u0027s not on the db row (i.e. leave it unset on the object) and not worry about any of these expensive tricks to find it.\n\nYou could put the expensive stuff in the load handler in case something tries to reference the un-set user_id field if you want,  in case something in the near future touches it, but you could also just log a \"FIXME\" so we would notice.","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a4c429125b9bb34a625d699db8ab700b2b31c510","unresolved":false,"context_lines":[{"line_number":127,"context_line":"                instance_mapping.user_id \u003d user_id"},{"line_number":128,"context_line":"            else:"},{"line_number":129,"context_line":"                raise exception.InstanceMappingNotFound("},{"line_number":130,"context_line":"                    uuid\u003dinstance_mapping.instance_uuid)"},{"line_number":131,"context_line":"        instance_mapping.obj_reset_changes()"},{"line_number":132,"context_line":"        instance_mapping._context \u003d context"},{"line_number":133,"context_line":"        return instance_mapping"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_4004d818","line":130,"in_reply_to":"9fdfeff1_2033d415","updated":"2019-03-04 22:48:49.000000000","message":"Actually, I\u0027m not sure I see where in the stack of patches on top of this that you need the actual user_id value set on the object. get_counts() just operates on the raw db records, right? Where is this actually used?","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"80e28e0121bad1d762321cb3735117249fd18808","unresolved":false,"context_lines":[{"line_number":127,"context_line":"                instance_mapping.user_id \u003d user_id"},{"line_number":128,"context_line":"            else:"},{"line_number":129,"context_line":"                raise exception.InstanceMappingNotFound("},{"line_number":130,"context_line":"                    uuid\u003dinstance_mapping.instance_uuid)"},{"line_number":131,"context_line":"        instance_mapping.obj_reset_changes()"},{"line_number":132,"context_line":"        instance_mapping._context \u003d context"},{"line_number":133,"context_line":"        return instance_mapping"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_b4106502","line":130,"in_reply_to":"9fdfeff1_4004d818","updated":"2019-03-04 23:12:07.000000000","message":"Yeah...it\u0027s weird. I didn\u0027t think on it enough yet. So yeah, agreed it should be a new exception instead.\n\nThat\u0027s correct that only the raw db records are considered for the user_id query. I didn\u0027t consider not adding the field to the object to match. The only places the user_id field is used is when creating InstanceMapping objects (and subsequently raw db records). We need to be able to pass user_id along for when InstanceMapping.create() happens.\n\nDid I miss something where we should consider doing that without adding user_id as a field on InstanceMapping?","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c7afdfa47acacc1bc755a5845e241bcabc77b200","unresolved":false,"context_lines":[{"line_number":127,"context_line":"                instance_mapping.user_id \u003d user_id"},{"line_number":128,"context_line":"            else:"},{"line_number":129,"context_line":"                raise exception.InstanceMappingNotFound("},{"line_number":130,"context_line":"                    uuid\u003dinstance_mapping.instance_uuid)"},{"line_number":131,"context_line":"        instance_mapping.obj_reset_changes()"},{"line_number":132,"context_line":"        instance_mapping._context \u003d context"},{"line_number":133,"context_line":"        return instance_mapping"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_32bb1876","line":130,"in_reply_to":"9fdfeff1_724520b4","updated":"2019-03-05 14:49:30.000000000","message":"Unless the goal is to do the online data migration right here and now on first read from the database when the user_id isn\u0027t set, but that can arguably be done in the API when showing an instance (as written in the spec) and the online data migration script.","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"c3e55eb5715b25e14f1ae25147802dc8f16224e0","unresolved":false,"context_lines":[{"line_number":127,"context_line":"                instance_mapping.user_id \u003d user_id"},{"line_number":128,"context_line":"            else:"},{"line_number":129,"context_line":"                raise exception.InstanceMappingNotFound("},{"line_number":130,"context_line":"                    uuid\u003dinstance_mapping.instance_uuid)"},{"line_number":131,"context_line":"        instance_mapping.obj_reset_changes()"},{"line_number":132,"context_line":"        instance_mapping._context \u003d context"},{"line_number":133,"context_line":"        return instance_mapping"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_14edb1ef","line":130,"in_reply_to":"9fdfeff1_b4106502","updated":"2019-03-04 23:16:43.000000000","message":"Or maybe you\u0027re just saying, maybe make _from_db_object skip the user_id field altogether and not set it at all?","commit_id":"0b56cdeb8a438246d45cbf5117f3cbbf4ba9355d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4813117bd15bbe8d27bf009bd44c62226581799b","unresolved":false,"context_lines":[{"line_number":62,"context_line":"                      \u0027mapping should not be accessed.\u0027)"},{"line_number":63,"context_line":"            raise exception.ObjectActionError("},{"line_number":64,"context_line":"                action\u003d\u0027obj_load_attr\u0027,"},{"line_number":65,"context_line":"                reason\u003d_(\u0027attribute user_id not lazy-loadable\u0027))"},{"line_number":66,"context_line":"        super(InstanceMapping, self).obj_load_attr(attrname)"},{"line_number":67,"context_line":""},{"line_number":68,"context_line":"    def _update_with_cell_id(self, updates):"}],"source_content_type":"text/x-python","patch_set":12,"id":"5fc1f717_8974369b","line":65,"range":{"start_line":65,"start_character":44,"end_line":65,"end_character":47},"updated":"2019-03-06 18:53:48.000000000","message":"nit: is not","commit_id":"2a12e420c2934e49da5cb3e586407d3f6e1d6300"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4813117bd15bbe8d27bf009bd44c62226581799b","unresolved":false,"context_lines":[{"line_number":63,"context_line":"            raise exception.ObjectActionError("},{"line_number":64,"context_line":"                action\u003d\u0027obj_load_attr\u0027,"},{"line_number":65,"context_line":"                reason\u003d_(\u0027attribute user_id not lazy-loadable\u0027))"},{"line_number":66,"context_line":"        super(InstanceMapping, self).obj_load_attr(attrname)"},{"line_number":67,"context_line":""},{"line_number":68,"context_line":"    def _update_with_cell_id(self, updates):"},{"line_number":69,"context_line":"        cell_mapping_obj \u003d updates.pop(\"cell_mapping\", None)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5fc1f717_c9ed5e26","line":66,"updated":"2019-03-06 18:53:48.000000000","message":"This fine, it will just raise NotImplementedError as normal.","commit_id":"2a12e420c2934e49da5cb3e586407d3f6e1d6300"}],"nova/objects/virtual_interface.py":[{"author":{"_account_id":26936,"name":"Surya Seetharaman","email":"suryaseetharaman.9@gmail.com","username":"tssurya"},"change_message_id":"25fa94a631267dc5efec12fe0e5192bcac20b79b","unresolved":false,"context_lines":[{"line_number":301,"context_line":"        instance \u003d objects.Instance(context)"},{"line_number":302,"context_line":"        instance.uuid \u003d FAKE_UUID"},{"line_number":303,"context_line":"        instance.project_id \u003d FAKE_UUID"},{"line_number":304,"context_line":"        instance.user_id \u003d FAKE_UUID"},{"line_number":305,"context_line":"        instance.create()"},{"line_number":306,"context_line":"        # Thats fake instance, lets destroy it."},{"line_number":307,"context_line":"        # We need only its row to solve constraint issue."}],"source_content_type":"text/x-python","patch_set":6,"id":"9fdfeff1_134927b6","line":304,"range":{"start_line":304,"start_character":8,"end_line":304,"end_character":36},"updated":"2019-02-21 13:16:32.000000000","message":"cool, so we have the marker\u0027s user_id updated with the FAKE_UUID here and also for the \"map_instances, so that we avoid infinite loops.","commit_id":"a59198ae4940f27551166cbaeddc8ceedb20ff24"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"29d0904bd913384cc71a37e5fd43903031e883df","unresolved":false,"context_lines":[{"line_number":301,"context_line":"        instance \u003d objects.Instance(context)"},{"line_number":302,"context_line":"        instance.uuid \u003d FAKE_UUID"},{"line_number":303,"context_line":"        instance.project_id \u003d FAKE_UUID"},{"line_number":304,"context_line":"        instance.user_id \u003d FAKE_UUID"},{"line_number":305,"context_line":"        instance.create()"},{"line_number":306,"context_line":"        # Thats fake instance, lets destroy it."},{"line_number":307,"context_line":"        # We need only its row to solve constraint issue."}],"source_content_type":"text/x-python","patch_set":6,"id":"9fdfeff1_397f6567","line":304,"range":{"start_line":304,"start_character":8,"end_line":304,"end_character":36},"in_reply_to":"9fdfeff1_134927b6","updated":"2019-02-21 17:12:11.000000000","message":"Yes. Thanks again for finding the root cause!","commit_id":"a59198ae4940f27551166cbaeddc8ceedb20ff24"}],"nova/tests/functional/db/test_instance_mapping.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4813117bd15bbe8d27bf009bd44c62226581799b","unresolved":false,"context_lines":[{"line_number":207,"context_line":"        self.assertEqual(0, total)"},{"line_number":208,"context_line":""},{"line_number":209,"context_line":""},{"line_number":210,"context_line":"class TestInstanceMappingUserId(test.TestCase):"},{"line_number":211,"context_line":"    def setUp(self):"},{"line_number":212,"context_line":"        super(TestInstanceMappingUserId, self).setUp()"},{"line_number":213,"context_line":"        # Use the same user and project as the sample mappings."}],"source_content_type":"text/x-python","patch_set":12,"id":"5fc1f717_29dc2273","line":210,"range":{"start_line":210,"start_character":6,"end_line":210,"end_character":31},"updated":"2019-03-06 18:53:48.000000000","message":"Why not just include the test in InstanceMappingTestCase?","commit_id":"2a12e420c2934e49da5cb3e586407d3f6e1d6300"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a0ecd2e464565fd5889fe5b7282381897c051180","unresolved":false,"context_lines":[{"line_number":207,"context_line":"        self.assertEqual(0, total)"},{"line_number":208,"context_line":""},{"line_number":209,"context_line":""},{"line_number":210,"context_line":"class TestInstanceMappingUserId(test.TestCase):"},{"line_number":211,"context_line":"    def setUp(self):"},{"line_number":212,"context_line":"        super(TestInstanceMappingUserId, self).setUp()"},{"line_number":213,"context_line":"        # Use the same user and project as the sample mappings."}],"source_content_type":"text/x-python","patch_set":12,"id":"5fc1f717_8913d60d","line":210,"range":{"start_line":210,"start_character":6,"end_line":210,"end_character":31},"in_reply_to":"5fc1f717_29dc2273","updated":"2019-03-06 18:59:32.000000000","message":"I don\u0027t think there\u0027s a good reason -- this is kinda residual from me initially modeling this after the BlockDeviceMapping.uuid migration/handling and wondering if handling of user_id is \"special\" and temporary and to separate it out because of that, possibly for easier removal later. I can move it into InstanceMappingTestCase.","commit_id":"2a12e420c2934e49da5cb3e586407d3f6e1d6300"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ee8f4cc44896ac377b1592acdfe23c5335b94c85","unresolved":false,"context_lines":[{"line_number":207,"context_line":"        self.assertEqual(0, total)"},{"line_number":208,"context_line":""},{"line_number":209,"context_line":""},{"line_number":210,"context_line":"class TestInstanceMappingUserId(test.TestCase):"},{"line_number":211,"context_line":"    def setUp(self):"},{"line_number":212,"context_line":"        super(TestInstanceMappingUserId, self).setUp()"},{"line_number":213,"context_line":"        # Use the same user and project as the sample mappings."}],"source_content_type":"text/x-python","patch_set":12,"id":"5fc1f717_d1a988df","line":210,"range":{"start_line":210,"start_character":6,"end_line":210,"end_character":31},"in_reply_to":"5fc1f717_8913d60d","updated":"2019-03-06 19:26:09.000000000","message":"OK if you\u0027re going to respin anyway just lump the new test into the existing class.","commit_id":"2a12e420c2934e49da5cb3e586407d3f6e1d6300"}],"nova/tests/unit/objects/test_instance_mapping.py":[{"author":{"_account_id":15888,"name":"Zhenyu Zheng","email":"zheng.zhenyu@outlook.com","username":"Kevin_Zheng"},"change_message_id":"bcd1d1d4aca33f6523de626c2b5f3a3b13000b40","unresolved":false,"context_lines":[{"line_number":162,"context_line":"        # Just ensure this doesn\u0027t raise an exception"},{"line_number":163,"context_line":"        mapping_obj.cell_mapping \u003d None"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    def test_obj_make_compatible(self):"},{"line_number":166,"context_line":"        uuid \u003d uuidutils.generate_uuid()"},{"line_number":167,"context_line":"        im_obj \u003d instance_mapping.InstanceMapping(context\u003dself.context)"},{"line_number":168,"context_line":"        fake_im_obj \u003d instance_mapping.InstanceMapping(context\u003dself.context,"},{"line_number":169,"context_line":"                                                       instance_uuid\u003duuid,"},{"line_number":170,"context_line":"                                                       queued_for_delete\u003dFalse,"},{"line_number":171,"context_line":"                                                       user_id\u003d\u0027fake-user\u0027)"},{"line_number":172,"context_line":"        obj_primitive \u003d fake_im_obj.obj_to_primitive(\u00271.0\u0027)"},{"line_number":173,"context_line":"        obj \u003d im_obj.obj_from_primitive(obj_primitive)"},{"line_number":174,"context_line":"        self.assertIn(\u0027instance_uuid\u0027, obj)"},{"line_number":175,"context_line":"        self.assertEqual(uuid, obj.instance_uuid)"},{"line_number":176,"context_line":"        self.assertNotIn(\u0027queued_for_delete\u0027, obj)"},{"line_number":177,"context_line":"        self.assertNotIn(\u0027user_id\u0027, obj)"},{"line_number":178,"context_line":""},{"line_number":179,"context_line":""},{"line_number":180,"context_line":"class TestInstanceMappingObject(test_objects._LocalTest,"}],"source_content_type":"text/x-python","patch_set":4,"id":"9fdfeff1_cf0e0b8b","line":177,"range":{"start_line":165,"start_character":4,"end_line":177,"end_character":40},"updated":"2019-02-15 03:25:00.000000000","message":"could be better to test all versions? like in \nhttps://review.openstack.org/#/c/614035/15/nova/tests/unit/objects/test_request_spec.py@819","commit_id":"0deb1fa105f3f7110acef95c7b12ece5930f29ca"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f68042747c9500757df5828ac016e3aeb50b45ab","unresolved":false,"context_lines":[{"line_number":162,"context_line":"        # Just ensure this doesn\u0027t raise an exception"},{"line_number":163,"context_line":"        mapping_obj.cell_mapping \u003d None"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"    def test_obj_make_compatible(self):"},{"line_number":166,"context_line":"        uuid \u003d uuidutils.generate_uuid()"},{"line_number":167,"context_line":"        im_obj \u003d instance_mapping.InstanceMapping(context\u003dself.context)"},{"line_number":168,"context_line":"        fake_im_obj \u003d instance_mapping.InstanceMapping(context\u003dself.context,"},{"line_number":169,"context_line":"                                                       instance_uuid\u003duuid,"},{"line_number":170,"context_line":"                                                       queued_for_delete\u003dFalse,"},{"line_number":171,"context_line":"                                                       user_id\u003d\u0027fake-user\u0027)"},{"line_number":172,"context_line":"        obj_primitive \u003d fake_im_obj.obj_to_primitive(\u00271.0\u0027)"},{"line_number":173,"context_line":"        obj \u003d im_obj.obj_from_primitive(obj_primitive)"},{"line_number":174,"context_line":"        self.assertIn(\u0027instance_uuid\u0027, obj)"},{"line_number":175,"context_line":"        self.assertEqual(uuid, obj.instance_uuid)"},{"line_number":176,"context_line":"        self.assertNotIn(\u0027queued_for_delete\u0027, obj)"},{"line_number":177,"context_line":"        self.assertNotIn(\u0027user_id\u0027, obj)"},{"line_number":178,"context_line":""},{"line_number":179,"context_line":""},{"line_number":180,"context_line":"class TestInstanceMappingObject(test_objects._LocalTest,"}],"source_content_type":"text/x-python","patch_set":4,"id":"9fdfeff1_228a3c63","line":177,"range":{"start_line":165,"start_character":4,"end_line":177,"end_character":40},"in_reply_to":"9fdfeff1_cf0e0b8b","updated":"2019-02-15 20:36:55.000000000","message":"I suppose so. Thanks for linking the example.","commit_id":"0deb1fa105f3f7110acef95c7b12ece5930f29ca"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"60ea3b219ecb8a2ca7ab45be2fc848ea06219e5c","unresolved":false,"context_lines":[{"line_number":107,"context_line":"                 \u0027queued_for_delete\u0027: False,"},{"line_number":108,"context_line":"                 \u0027project_id\u0027: db_mapping[\u0027project_id\u0027],"},{"line_number":109,"context_line":"                 \u0027user_id\u0027: self.context.user_id})"},{"line_number":110,"context_line":"        self.compare_obj(mapping_obj, db_mapping,"},{"line_number":111,"context_line":"                         subs\u003d{\u0027cell_mapping\u0027: \u0027cell_id\u0027})"},{"line_number":112,"context_line":"        self.assertIsNone(mapping_obj.cell_mapping)"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    @mock.patch.object(instance_mapping.InstanceMapping, \u0027_create_in_db\u0027)"}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_cb092d31","line":111,"range":{"start_line":110,"start_character":8,"end_line":111,"end_character":58},"updated":"2019-02-27 19:07:10.000000000","message":"You\u0027re getting lucky here that self.context.user_id is \u0027fake-user\u0027 from _BaseTestCase and you used \u0027fake-user\u0027 in get_db_mapping(). I\u0027m not asking you to change it, just pointing it out.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"230ee439122b3021ada5cb1062dd34daa7b27ef9","unresolved":false,"context_lines":[{"line_number":107,"context_line":"                 \u0027queued_for_delete\u0027: False,"},{"line_number":108,"context_line":"                 \u0027project_id\u0027: db_mapping[\u0027project_id\u0027],"},{"line_number":109,"context_line":"                 \u0027user_id\u0027: self.context.user_id})"},{"line_number":110,"context_line":"        self.compare_obj(mapping_obj, db_mapping,"},{"line_number":111,"context_line":"                         subs\u003d{\u0027cell_mapping\u0027: \u0027cell_id\u0027})"},{"line_number":112,"context_line":"        self.assertIsNone(mapping_obj.cell_mapping)"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    @mock.patch.object(instance_mapping.InstanceMapping, \u0027_create_in_db\u0027)"}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_56d63d51","line":111,"range":{"start_line":110,"start_character":8,"end_line":111,"end_character":58},"in_reply_to":"9fdfeff1_cb092d31","updated":"2019-02-27 22:41:22.000000000","message":"I did check the origin of the user_id to make sure it would work right. I\u0027ll add a comment explaining why it works. Or I could set the user_id in the test to make it obvious.","commit_id":"cfc1cb218c2f4824819ecd36c391a251995f3a16"}]}