)]}'
{"nova/objects/instance_mapping.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c3eba3a8613de017ab84f9c5cbdeb9f589b953a0","unresolved":false,"context_lines":[{"line_number":399,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":400,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":401,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":402,"context_line":"        project_result \u003d project_query.first()"},{"line_number":403,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: int(project_result[0] or 0)}}"},{"line_number":404,"context_line":"        if user_id:"},{"line_number":405,"context_line":"            user_result \u003d project_query.filter_by(user_id\u003duser_id).first()"}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_6c7db7b0","line":402,"range":{"start_line":402,"start_character":39,"end_line":402,"end_character":44},"updated":"2019-02-27 21:03:36.000000000","message":"use scalar for sanity","commit_id":"c89a82eaee9d6ec4fb349de72fc643414b0af799"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"fadb2f2162c850410383608442d152024dfe9748","unresolved":false,"context_lines":[{"line_number":399,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":400,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":401,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":402,"context_line":"        project_result \u003d project_query.first()"},{"line_number":403,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: int(project_result[0] or 0)}}"},{"line_number":404,"context_line":"        if user_id:"},{"line_number":405,"context_line":"            user_result \u003d project_query.filter_by(user_id\u003duser_id).first()"}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_b6b20993","line":402,"range":{"start_line":402,"start_character":39,"end_line":402,"end_character":44},"in_reply_to":"9fdfeff1_6c7db7b0","updated":"2019-02-27 23:12:31.000000000","message":"Will do.","commit_id":"c89a82eaee9d6ec4fb349de72fc643414b0af799"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c3eba3a8613de017ab84f9c5cbdeb9f589b953a0","unresolved":false,"context_lines":[{"line_number":402,"context_line":"        project_result \u003d project_query.first()"},{"line_number":403,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: int(project_result[0] or 0)}}"},{"line_number":404,"context_line":"        if user_id:"},{"line_number":405,"context_line":"            user_result \u003d project_query.filter_by(user_id\u003duser_id).first()"},{"line_number":406,"context_line":"            counts[\u0027user\u0027] \u003d {\u0027instances\u0027: int(user_result[0] or 0)}"},{"line_number":407,"context_line":"        return counts"},{"line_number":408,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_8c781bc1","line":405,"range":{"start_line":405,"start_character":67,"end_line":405,"end_character":72},"updated":"2019-02-27 21:03:36.000000000","message":"same","commit_id":"c89a82eaee9d6ec4fb349de72fc643414b0af799"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c3eba3a8613de017ab84f9c5cbdeb9f589b953a0","unresolved":false,"context_lines":[{"line_number":410,"context_line":"    def get_counts(cls, context, project_id, user_id\u003dNone):"},{"line_number":411,"context_line":"        \"\"\"Get the counts of InstanceMapping objects in the database."},{"line_number":412,"context_line":""},{"line_number":413,"context_line":"        The count is used to represent the count of instances for the purpose"},{"line_number":414,"context_line":"        of counting quota usage."},{"line_number":415,"context_line":""},{"line_number":416,"context_line":"        :param context: The request context for database access"}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_8c51fb32","line":413,"updated":"2019-02-27 21:03:36.000000000","message":"Might want to mention this does not count queued_for_delete\u003dTrue mappings.","commit_id":"c89a82eaee9d6ec4fb349de72fc643414b0af799"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"fadb2f2162c850410383608442d152024dfe9748","unresolved":false,"context_lines":[{"line_number":410,"context_line":"    def get_counts(cls, context, project_id, user_id\u003dNone):"},{"line_number":411,"context_line":"        \"\"\"Get the counts of InstanceMapping objects in the database."},{"line_number":412,"context_line":""},{"line_number":413,"context_line":"        The count is used to represent the count of instances for the purpose"},{"line_number":414,"context_line":"        of counting quota usage."},{"line_number":415,"context_line":""},{"line_number":416,"context_line":"        :param context: The request context for database access"}],"source_content_type":"text/x-python","patch_set":7,"id":"9fdfeff1_d6ad4d31","line":413,"in_reply_to":"9fdfeff1_8c51fb32","updated":"2019-02-27 23:12:31.000000000","message":"Will add.","commit_id":"c89a82eaee9d6ec4fb349de72fc643414b0af799"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"3c645f69c1978b746eb97fb648502161f6b07659","unresolved":false,"context_lines":[{"line_number":384,"context_line":"        \"\"\"Get the counts of InstanceMapping objects in the database."},{"line_number":385,"context_line":""},{"line_number":386,"context_line":"        The count is used to represent the count of instances for the purpose"},{"line_number":387,"context_line":"        of counting quota usage."},{"line_number":388,"context_line":""},{"line_number":389,"context_line":"        :param context: The request context for database access"},{"line_number":390,"context_line":"        :param project_id: The project_id to count across"}],"source_content_type":"text/x-python","patch_set":9,"id":"9fdfeff1_9cdcad45","line":387,"updated":"2019-02-28 17:46:03.000000000","message":"I forgot to add mention of the fact that this count does not consider queued_for_delete\u003dTrue mappings. Add on the next respin.","commit_id":"89e8888a5d44cdf28899dccb74aae11c00f83833"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b73124825a220fa2397fd7abe470dbb3b3464610","unresolved":false,"context_lines":[{"line_number":348,"context_line":"            # Note that the project_id can be None in case"},{"line_number":349,"context_line":"            # instances are being listed for the all-tenants case."},{"line_number":350,"context_line":"            query \u003d query.filter_by(project_id\u003dproject_id)"},{"line_number":351,"context_line":"        # Both the values NULL (for cases when the online data migration for"},{"line_number":352,"context_line":"        # queued_for_delete was not run) and False (cases when the online"},{"line_number":353,"context_line":"        # data migration for queued_for_delete was run) are assumed to mean"},{"line_number":354,"context_line":"        # that the instance is not queued for deletion."},{"line_number":355,"context_line":"        query \u003d (query.filter(or_("},{"line_number":356,"context_line":"            api_models.InstanceMapping.queued_for_delete \u003d\u003d false(),"},{"line_number":357,"context_line":"            api_models.InstanceMapping.queued_for_delete.is_(None)))"},{"line_number":358,"context_line":"            .join(\u0027cell_mapping\u0027)"},{"line_number":359,"context_line":"            .options(joinedload(\u0027cell_mapping\u0027))"},{"line_number":360,"context_line":"            .filter(api_models.CellMapping.uuid \u003d\u003d cell_uuid))"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_d1450819","line":357,"range":{"start_line":351,"start_character":8,"end_line":357,"end_character":68},"updated":"2019-03-06 19:31:30.000000000","message":"See below.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b73124825a220fa2397fd7abe470dbb3b3464610","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":384,"context_line":"        project_query \u003d context.session.query("},{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_31bfacfc","line":386,"range":{"start_line":386,"start_character":12,"end_line":386,"end_character":48},"updated":"2019-03-06 19:31:30.000000000","message":"If we have instance mappings that don\u0027t yet have queued_for_delete migrated, those will be filtered out of this query but could still exist - we don\u0027t really know if the backing instance is deleted or not though...so I\u0027m not sure if we just shrug this off or what. Surya had to deal with this in _get_not_deleted_by_cell_and_project_from_db above so maybe see what she says.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ded14abf6dae862fa6fd7c6d1993ca89fb229392","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":384,"context_line":"        project_query \u003d context.session.query("},{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_91baae07","line":386,"range":{"start_line":386,"start_character":12,"end_line":386,"end_character":48},"in_reply_to":"5fc1f717_11671050","updated":"2019-03-12 20:57:17.000000000","message":"OK, having a look at this again, I see now why I didn\u0027t count unmigrated records. It\u0027s because, if you take a look at the next two patches in the series, we will detect the presence of unmigrated records using \u0027user_id_populated\u0027 and if any are found, fall back on the legacy counting method. We will not use the InstanceMappingList count if there are any unmigrated records for a project (or any project, in the case of server group members).","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"74721fa215bb64f4eee9f5aedae33ef9be29bba5","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":384,"context_line":"        project_query \u003d context.session.query("},{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":14,"id":"3fce034c_2c821351","line":386,"range":{"start_line":386,"start_character":12,"end_line":386,"end_character":48},"in_reply_to":"5fc1f717_1c96854a","updated":"2019-04-10 19:11:47.000000000","message":"\u003e \u003e That\u0027s fair enough. I\u0027ll add a comment to the code next respin.\n \u003e \u003e Unless you would prefer I do it now -- I was thinking not to\n \u003e churn\n \u003e \u003e these patches while they\u0027re still being looked at.\n \u003e \n \u003e I\u0027ll move on to the next patch in the series to try and load the\n \u003e context into my head about how this is going to be used.\n\nBased on my discussion with Surya, instead of counting unmigrated queued_for_delete mappings (and potentially being inaccurate for quota checking), I\u0027m going to fall back on legacy counting if any unmigrated queued_for_delete mappings are detected (in the actual counting patch above this one).","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2b1a4a71d35d52757bf1f4607247e08212b001a7","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":384,"context_line":"        project_query \u003d context.session.query("},{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_f1b7e4d7","line":386,"range":{"start_line":386,"start_character":12,"end_line":386,"end_character":48},"in_reply_to":"5fc1f717_31bfacfc","updated":"2019-03-06 19:36:30.000000000","message":"I tend to think we should count those with queued_for_delete\u003dNone since we don\u0027t know if the instance is deleted so we should opt into being conservative.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":26936,"name":"Surya Seetharaman","email":"suryaseetharaman.9@gmail.com","username":"tssurya"},"change_message_id":"12cb7d682f0a9cf222d5cd678886214feca03d47","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":384,"context_line":"        project_query \u003d context.session.query("},{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_dc79dd92","line":386,"range":{"start_line":386,"start_character":12,"end_line":386,"end_character":48},"in_reply_to":"5fc1f717_4c7464fe","updated":"2019-04-10 18:47:18.000000000","message":"\u003e OK, thanks for this input. So instead of counting queued_for_delete\u003dNone,\n \u003e we could fall back on legacy counting if any queued_for_delete\u003dNone\n \u003e are detected.\n \u003e \n \u003e I\u0027ll update the user_id_populated method \u003d\u003e user_id_queued_for_delete_populated\n \u003e and use that as the fall back logic.\n\nthanks for changing this","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"60e19fbc876315a0e908e1ce8752cd0ecf6b8325","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":384,"context_line":"        project_query \u003d context.session.query("},{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_995b6784","line":386,"range":{"start_line":386,"start_character":12,"end_line":386,"end_character":48},"in_reply_to":"5fc1f717_4c7464fe","updated":"2019-04-10 18:21:21.000000000","message":"Looking back on this after a month, I see no difference in code from PS14 but I would have expected at least a comment in the code or the commit message about why this doesn\u0027t consider unmigrated instance mappings as part of the count so we don\u0027t have to re-learn this from the review discussion if looking at the code.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"613a4064c8301309c47cfb5a2b60c8100c9a1342","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":384,"context_line":"        project_query \u003d context.session.query("},{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_1c96854a","line":386,"range":{"start_line":386,"start_character":12,"end_line":386,"end_character":48},"in_reply_to":"5fc1f717_7c05490f","updated":"2019-04-10 19:00:13.000000000","message":"\u003e That\u0027s fair enough. I\u0027ll add a comment to the code next respin.\n \u003e Unless you would prefer I do it now -- I was thinking not to churn\n \u003e these patches while they\u0027re still being looked at.\n\nI\u0027ll move on to the next patch in the series to try and load the context into my head about how this is going to be used.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"fe322692530d6925b1003bf4b10137473c1a8586","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":384,"context_line":"        project_query \u003d context.session.query("},{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_4c7464fe","line":386,"range":{"start_line":386,"start_character":12,"end_line":386,"end_character":48},"in_reply_to":"5fc1f717_7fed89b5","updated":"2019-03-27 18:33:57.000000000","message":"OK, thanks for this input. So instead of counting queued_for_delete\u003dNone, we could fall back on legacy counting if any queued_for_delete\u003dNone are detected.\n\nI did notice in the populate_queued_for_delete method, it will set queued_for_delete\u003dFalse for any orphan instance mapping records. So it would seem they would be counted for quota. BUT...\n\nIn the populate_user_id method, we don\u0027t attempt to migrate orphan instance mapping records and they remain with user_id\u003dNone, so they would not be used in quota usage counting. So I think this covers the queued_for_delete\u003dFalse for orphan records concern from my previous paragraph. If any orphan records are present, we\u0027ll fall back to legacy quota usage counting.\n\nI\u0027ll update the user_id_populated method \u003d\u003e user_id_queued_for_delete_populated and use that as the fall back logic.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":26936,"name":"Surya Seetharaman","email":"suryaseetharaman.9@gmail.com","username":"tssurya"},"change_message_id":"5330ce207e099e587864f7ff40db8f4de62e39dd","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":384,"context_line":"        project_query \u003d context.session.query("},{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_7fed89b5","line":386,"range":{"start_line":386,"start_character":12,"end_line":386,"end_character":48},"in_reply_to":"5fc1f717_91baae07","updated":"2019-03-25 12:19:13.000000000","message":"\u003e OK, having a look at this again, I see now why I didn\u0027t count\n \u003e unmigrated records. It\u0027s because, if you take a look at the next\n \u003e two patches in the series, we will detect the presence of\n \u003e unmigrated records using \u0027user_id_populated\u0027 and if any are found,\n \u003e fall back on the legacy counting method. We will not use the\n \u003e InstanceMappingList count if there are any unmigrated records for a\n \u003e project (or any project, in the case of server group members).\n\nI feel we should do the same for non-migrated queued_for_delete as well. i.e if queued_for_delete\u003dNone we should fall back to legacy, while being conservative is okay, counting them and having users complain about quota when they have them would be bad,","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"8fa5cb7c312d228c06f963bf755eb15cdebafb8c","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":384,"context_line":"        project_query \u003d context.session.query("},{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_7c05490f","line":386,"range":{"start_line":386,"start_character":12,"end_line":386,"end_character":48},"in_reply_to":"5fc1f717_995b6784","updated":"2019-04-10 18:37:12.000000000","message":"That\u0027s fair enough. I\u0027ll add a comment to the code next respin. Unless you would prefer I do it now -- I was thinking not to churn these patches while they\u0027re still being looked at.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a177a1a6acef0d91ea9d20381ccf8a56ddfbce10","unresolved":false,"context_lines":[{"line_number":383,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":384,"context_line":"        project_query \u003d context.session.query("},{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_11671050","line":386,"range":{"start_line":386,"start_character":12,"end_line":386,"end_character":48},"in_reply_to":"5fc1f717_f1b7e4d7","updated":"2019-03-06 19:43:07.000000000","message":"Yeah, good point, I didn\u0027t consider the unmigrated records possibility. I\u0027d tend to agree to be conservative and count them. Let\u0027s see what Surya thinks.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b73124825a220fa2397fd7abe470dbb3b3464610","unresolved":false,"context_lines":[{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"},{"line_number":390,"context_line":"        if user_id:"},{"line_number":391,"context_line":"            user_result \u003d project_query.filter_by(user_id\u003duser_id).scalar()"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_31e6cc46","line":388,"range":{"start_line":388,"start_character":39,"end_line":388,"end_character":45},"updated":"2019-03-06 19:31:30.000000000","message":"Gotta love that hot scalar action.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a177a1a6acef0d91ea9d20381ccf8a56ddfbce10","unresolved":false,"context_lines":[{"line_number":385,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":386,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":387,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":388,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":389,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"},{"line_number":390,"context_line":"        if user_id:"},{"line_number":391,"context_line":"            user_result \u003d project_query.filter_by(user_id\u003duser_id).scalar()"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_b15b9c85","line":388,"range":{"start_line":388,"start_character":39,"end_line":388,"end_character":45},"in_reply_to":"5fc1f717_31e6cc46","updated":"2019-03-06 19:43:07.000000000","message":"Hah.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":26936,"name":"Surya Seetharaman","email":"suryaseetharaman.9@gmail.com","username":"tssurya"},"change_message_id":"5330ce207e099e587864f7ff40db8f4de62e39dd","unresolved":false,"context_lines":[{"line_number":399,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":400,"context_line":"        project_query \u003d context.session.query("},{"line_number":401,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":402,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":403,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":404,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":405,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":18,"id":"5fc1f717_f4c8c0a8","line":402,"range":{"start_line":402,"start_character":22,"end_line":402,"end_character":45},"updated":"2019-03-25 12:19:13.000000000","message":"just confirming: we don\u0027t need to count the resources for soft-deleted instances right ? as in soft-deleted frees up resources like shelving right ?","commit_id":"8ea264dc95ab6c1b03d3fc600ddf11149aa519d3"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"fe322692530d6925b1003bf4b10137473c1a8586","unresolved":false,"context_lines":[{"line_number":399,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":400,"context_line":"        project_query \u003d context.session.query("},{"line_number":401,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":402,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":403,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":404,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":405,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":18,"id":"5fc1f717_4c7ea4af","line":402,"range":{"start_line":402,"start_character":22,"end_line":402,"end_character":45},"in_reply_to":"5fc1f717_f4c8c0a8","updated":"2019-03-27 18:33:57.000000000","message":"Right, today SOFT_DELETED instances do not count against a user\u0027s quota [1]. So they should not be counted for quota usage.\n\n[1] https://github.com/openstack/nova/blob/38ce9d8/nova/objects/instance.py#L1540","commit_id":"8ea264dc95ab6c1b03d3fc600ddf11149aa519d3"},{"author":{"_account_id":26936,"name":"Surya Seetharaman","email":"suryaseetharaman.9@gmail.com","username":"tssurya"},"change_message_id":"5330ce207e099e587864f7ff40db8f4de62e39dd","unresolved":false,"context_lines":[{"line_number":404,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":405,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"},{"line_number":406,"context_line":"        if user_id:"},{"line_number":407,"context_line":"            user_result \u003d project_query.filter_by(user_id\u003duser_id).scalar()"},{"line_number":408,"context_line":"            counts[\u0027user\u0027] \u003d {\u0027instances\u0027: user_result}"},{"line_number":409,"context_line":"        return counts"},{"line_number":410,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"5fc1f717_7f1589e4","line":407,"range":{"start_line":407,"start_character":26,"end_line":407,"end_character":66},"updated":"2019-03-25 12:19:13.000000000","message":"self note: okay so user based quotas are always per project per user.","commit_id":"8ea264dc95ab6c1b03d3fc600ddf11149aa519d3"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"fe322692530d6925b1003bf4b10137473c1a8586","unresolved":false,"context_lines":[{"line_number":404,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":405,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"},{"line_number":406,"context_line":"        if user_id:"},{"line_number":407,"context_line":"            user_result \u003d project_query.filter_by(user_id\u003duser_id).scalar()"},{"line_number":408,"context_line":"            counts[\u0027user\u0027] \u003d {\u0027instances\u0027: user_result}"},{"line_number":409,"context_line":"        return counts"},{"line_number":410,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"5fc1f717_cc43746b","line":407,"range":{"start_line":407,"start_character":26,"end_line":407,"end_character":66},"in_reply_to":"5fc1f717_7f1589e4","updated":"2019-03-27 18:33:57.000000000","message":"Correct. It\u0027s either project, or project + user.","commit_id":"8ea264dc95ab6c1b03d3fc600ddf11149aa519d3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ecd0d678795dc2f80db8f7b6319b5cec584b46d4","unresolved":false,"context_lines":[{"line_number":399,"context_line":"    def _get_counts_in_db(context, project_id, user_id\u003dNone):"},{"line_number":400,"context_line":"        project_query \u003d context.session.query("},{"line_number":401,"context_line":"            func.count(api_models.InstanceMapping.id)).\\"},{"line_number":402,"context_line":"            filter_by(queued_for_delete\u003dFalse).\\"},{"line_number":403,"context_line":"            filter_by(project_id\u003dproject_id)"},{"line_number":404,"context_line":"        project_result \u003d project_query.scalar()"},{"line_number":405,"context_line":"        counts \u003d {\u0027project\u0027: {\u0027instances\u0027: project_result}}"}],"source_content_type":"text/x-python","patch_set":20,"id":"bfb3d3c7_b4b45081","line":402,"range":{"start_line":402,"start_character":40,"end_line":402,"end_character":45},"updated":"2019-05-29 13:58:24.000000000","message":"nit: could have used the false() sqla function here like above in _get_not_deleted_by_cell_and_project_from_db.","commit_id":"85e287d3ef698548c03c635a81d3b60b6ebe9ba8"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ecd0d678795dc2f80db8f7b6319b5cec584b46d4","unresolved":false,"context_lines":[{"line_number":417,"context_line":"        not included in the count (deleted and SOFT_DELETED instances)."},{"line_number":418,"context_line":"        Instances that are queued_for_deleted\u003dNone are not included in the"},{"line_number":419,"context_line":"        count because we are not certain about whether or not they are deleted."},{"line_number":420,"context_line":"        When counting quota usage, we will fall back on the legacy counting"},{"line_number":421,"context_line":"        method and count instances, cores, and ram from cell databases if any"},{"line_number":422,"context_line":"        unmigrated instance mappings (user_id\u003dNone or queued_for_delete\u003dNone)"},{"line_number":423,"context_line":"        are detected, to avoid using a potentially inaccurate count."},{"line_number":424,"context_line":""},{"line_number":425,"context_line":"        :param context: The request context for database access"},{"line_number":426,"context_line":"        :param project_id: The project_id to count across"}],"source_content_type":"text/x-python","patch_set":20,"id":"bfb3d3c7_14a19c46","line":423,"range":{"start_line":420,"start_character":8,"end_line":423,"end_character":68},"updated":"2019-05-29 13:58:24.000000000","message":"nit: this doesn\u0027t seem appropriate content for this method\u0027s docstring - this is just a tool, how it\u0027s used is out of the scope of this method for the most part (could go elsewhere when calling this or in the commit message).","commit_id":"85e287d3ef698548c03c635a81d3b60b6ebe9ba8"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9da2d317feef85856104551ab429752de6af2ac2","unresolved":false,"context_lines":[{"line_number":417,"context_line":"        not included in the count (deleted and SOFT_DELETED instances)."},{"line_number":418,"context_line":"        Instances that are queued_for_deleted\u003dNone are not included in the"},{"line_number":419,"context_line":"        count because we are not certain about whether or not they are deleted."},{"line_number":420,"context_line":"        When counting quota usage, we will fall back on the legacy counting"},{"line_number":421,"context_line":"        method and count instances, cores, and ram from cell databases if any"},{"line_number":422,"context_line":"        unmigrated instance mappings (user_id\u003dNone or queued_for_delete\u003dNone)"},{"line_number":423,"context_line":"        are detected, to avoid using a potentially inaccurate count."},{"line_number":424,"context_line":""},{"line_number":425,"context_line":"        :param context: The request context for database access"},{"line_number":426,"context_line":"        :param project_id: The project_id to count across"}],"source_content_type":"text/x-python","patch_set":20,"id":"bfb3d3c7_6f1b6572","line":423,"range":{"start_line":420,"start_character":8,"end_line":423,"end_character":68},"in_reply_to":"bfb3d3c7_14a19c46","updated":"2019-05-29 13:59:45.000000000","message":"Looking back I see this was my fault, I asked for a comment about unmigrated instances in the code or commit message:\n\nhttps://review.opendev.org/#/c/638072/14/nova/objects/instance_mapping.py@386","commit_id":"85e287d3ef698548c03c635a81d3b60b6ebe9ba8"}],"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":"b73124825a220fa2397fd7abe470dbb3b3464610","unresolved":false,"context_lines":[{"line_number":522,"context_line":"                                                   None))"},{"line_number":523,"context_line":"        self.assertEqual(2, len(ims))"},{"line_number":524,"context_line":""},{"line_number":525,"context_line":"    def test_get_counts(self):"},{"line_number":526,"context_line":"        create_mapping(project_id\u003d\u0027fake-project\u0027, user_id\u003d\u0027fake-user\u0027,"},{"line_number":527,"context_line":"                       queued_for_delete\u003dFalse)"},{"line_number":528,"context_line":"        # mapping with another user"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_d15e68e5","line":525,"updated":"2019-03-06 19:31:30.000000000","message":"We should probably also have an instance mapping with queud_for_delete\u003dNone.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a177a1a6acef0d91ea9d20381ccf8a56ddfbce10","unresolved":false,"context_lines":[{"line_number":522,"context_line":"                                                   None))"},{"line_number":523,"context_line":"        self.assertEqual(2, len(ims))"},{"line_number":524,"context_line":""},{"line_number":525,"context_line":"    def test_get_counts(self):"},{"line_number":526,"context_line":"        create_mapping(project_id\u003d\u0027fake-project\u0027, user_id\u003d\u0027fake-user\u0027,"},{"line_number":527,"context_line":"                       queued_for_delete\u003dFalse)"},{"line_number":528,"context_line":"        # mapping with another user"}],"source_content_type":"text/x-python","patch_set":14,"id":"5fc1f717_d160a835","line":525,"in_reply_to":"5fc1f717_d15e68e5","updated":"2019-03-06 19:43:07.000000000","message":"Ack.","commit_id":"d07d177d42751fe2b911f371e8d121f5e6ab4887"},{"author":{"_account_id":15888,"name":"Zhenyu Zheng","email":"zheng.zhenyu@outlook.com","username":"Kevin_Zheng"},"change_message_id":"4081018628a27c6600bfc39e923a955ae66c8ef3","unresolved":false,"context_lines":[{"line_number":590,"context_line":"        # queued_for_delete\u003dTrue, should not be counted"},{"line_number":591,"context_line":"        create_mapping(project_id\u003d\u0027fake-project\u0027, user_id\u003d\u0027fake-user\u0027,"},{"line_number":592,"context_line":"                       queued_for_delete\u003dTrue)"},{"line_number":593,"context_line":"        # queued_for_delete\u003dNone (not yet migrated), should not be counted"},{"line_number":594,"context_line":"        create_mapping(project_id\u003d\u0027fake-project\u0027, user_id\u003d\u0027fake-user\u0027,"},{"line_number":595,"context_line":"                       queued_for_delete\u003dNone)"},{"line_number":596,"context_line":""},{"line_number":597,"context_line":"        # Count only across a project"},{"line_number":598,"context_line":"        counts \u003d instance_mapping.InstanceMappingList.get_counts("}],"source_content_type":"text/x-python","patch_set":20,"id":"ffb9cba7_456f19c5","line":595,"range":{"start_line":593,"start_character":8,"end_line":595,"end_character":46},"updated":"2019-04-22 02:23:05.000000000","message":"just a little confusing here since according to the workflow, we shouldn\u0027t even get here if queued_for_delete\u003dNone, but it is also true that we will exclude the record if queued_for_delete\u003dNone","commit_id":"85e287d3ef698548c03c635a81d3b60b6ebe9ba8"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"72439596ba9f5b971e9f2153461cec39d018f185","unresolved":false,"context_lines":[{"line_number":590,"context_line":"        # queued_for_delete\u003dTrue, should not be counted"},{"line_number":591,"context_line":"        create_mapping(project_id\u003d\u0027fake-project\u0027, user_id\u003d\u0027fake-user\u0027,"},{"line_number":592,"context_line":"                       queued_for_delete\u003dTrue)"},{"line_number":593,"context_line":"        # queued_for_delete\u003dNone (not yet migrated), should not be counted"},{"line_number":594,"context_line":"        create_mapping(project_id\u003d\u0027fake-project\u0027, user_id\u003d\u0027fake-user\u0027,"},{"line_number":595,"context_line":"                       queued_for_delete\u003dNone)"},{"line_number":596,"context_line":""},{"line_number":597,"context_line":"        # Count only across a project"},{"line_number":598,"context_line":"        counts \u003d instance_mapping.InstanceMappingList.get_counts("}],"source_content_type":"text/x-python","patch_set":20,"id":"bfb3d3c7_a7b98115","line":595,"range":{"start_line":593,"start_character":8,"end_line":595,"end_character":46},"in_reply_to":"bfb3d3c7_b4621009","updated":"2019-05-29 15:42:25.000000000","message":"Ah, thank you. I\u0027ll propose a follow up patch to remove that bit of the docstring and address other nits.","commit_id":"85e287d3ef698548c03c635a81d3b60b6ebe9ba8"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ecd0d678795dc2f80db8f7b6319b5cec584b46d4","unresolved":false,"context_lines":[{"line_number":590,"context_line":"        # queued_for_delete\u003dTrue, should not be counted"},{"line_number":591,"context_line":"        create_mapping(project_id\u003d\u0027fake-project\u0027, user_id\u003d\u0027fake-user\u0027,"},{"line_number":592,"context_line":"                       queued_for_delete\u003dTrue)"},{"line_number":593,"context_line":"        # queued_for_delete\u003dNone (not yet migrated), should not be counted"},{"line_number":594,"context_line":"        create_mapping(project_id\u003d\u0027fake-project\u0027, user_id\u003d\u0027fake-user\u0027,"},{"line_number":595,"context_line":"                       queued_for_delete\u003dNone)"},{"line_number":596,"context_line":""},{"line_number":597,"context_line":"        # Count only across a project"},{"line_number":598,"context_line":"        counts \u003d instance_mapping.InstanceMappingList.get_counts("}],"source_content_type":"text/x-python","patch_set":20,"id":"bfb3d3c7_b4621009","line":595,"range":{"start_line":593,"start_character":8,"end_line":595,"end_character":46},"in_reply_to":"dfbec78f_23110105","updated":"2019-05-29 13:58:24.000000000","message":"Kevin is referring to the thing I commented on in the docstring of the method:\n\n\u003e When counting quota usage, we will fall back on the legacy counting method and count instances, cores, and ram from cell databases if any unmigrated instance mappings (user_id\u003dNone or queued_for_delete\u003dNone) are detected, to avoid using a potentially inaccurate count.\n\nMeaning if there are unmigrated instances that says the method won\u0027t even be called. Anyway, I think it\u0027s fine to test this scenario for how the counting function works, but as noted the docstring is a bit out of place since it shouldn\u0027t itself be concerned with who or what is calling it and why (you\u0027re tightly coupling the method to what uses it).","commit_id":"85e287d3ef698548c03c635a81d3b60b6ebe9ba8"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4438080cb789571614f7888cc4ca3603efde8574","unresolved":false,"context_lines":[{"line_number":590,"context_line":"        # queued_for_delete\u003dTrue, should not be counted"},{"line_number":591,"context_line":"        create_mapping(project_id\u003d\u0027fake-project\u0027, user_id\u003d\u0027fake-user\u0027,"},{"line_number":592,"context_line":"                       queued_for_delete\u003dTrue)"},{"line_number":593,"context_line":"        # queued_for_delete\u003dNone (not yet migrated), should not be counted"},{"line_number":594,"context_line":"        create_mapping(project_id\u003d\u0027fake-project\u0027, user_id\u003d\u0027fake-user\u0027,"},{"line_number":595,"context_line":"                       queued_for_delete\u003dNone)"},{"line_number":596,"context_line":""},{"line_number":597,"context_line":"        # Count only across a project"},{"line_number":598,"context_line":"        counts \u003d instance_mapping.InstanceMappingList.get_counts("}],"source_content_type":"text/x-python","patch_set":20,"id":"dfbec78f_23110105","line":595,"range":{"start_line":593,"start_character":8,"end_line":595,"end_character":46},"in_reply_to":"ffb9cba7_456f19c5","updated":"2019-05-15 16:25:56.000000000","message":"I don\u0027t think I follow what you mean here. This mapping is meant to simulate an instance mapping record that has not yet completed the migration for populating queued_for_delete (separate migration that is not part of this patch series).","commit_id":"85e287d3ef698548c03c635a81d3b60b6ebe9ba8"}]}