)]}'
{"neutron/db/quota/api.py":[{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"3f9d150fffa33a267f07e3dbdc6424d2a1ef0f1a","unresolved":true,"context_lines":[{"line_number":218,"context_line":"            set_resources_quota_usage_dirty(context, resources, tenant_id)"},{"line_number":219,"context_line":"        return 1"},{"line_number":220,"context_line":"    except db_exc.DBError:"},{"line_number":221,"context_line":"        context.session.rollback()"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"@db_api.retry_if_session_inactive()"}],"source_content_type":"text/x-python","patch_set":1,"id":"8fdaa31a_24668408","line":221,"range":{"start_line":221,"start_character":8,"end_line":221,"end_character":34},"updated":"2021-09-17 09:44:48.000000000","message":"Won\u0027t it rollback more than we expect?","commit_id":"f8f50397ca1e4ab7f5f31b19dde255ab70b4ccaf"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"c2c397fbeaeac35de90b6a32c2b5bb6065471b84","unresolved":true,"context_lines":[{"line_number":218,"context_line":"            set_resources_quota_usage_dirty(context, resources, tenant_id)"},{"line_number":219,"context_line":"        return 1"},{"line_number":220,"context_line":"    except db_exc.DBError:"},{"line_number":221,"context_line":"        context.session.rollback()"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"@db_api.retry_if_session_inactive()"}],"source_content_type":"text/x-python","patch_set":1,"id":"17936f17_901ef0cd","line":221,"range":{"start_line":221,"start_character":8,"end_line":221,"end_character":34},"in_reply_to":"12227701_22514ee2","updated":"2021-09-17 12:04:23.000000000","message":"hmm, but if any error will happen up to this point in transaction, we will not be able to commit it without rolling back. I don\u0027t think if there is other way to do it. Any ideas?","commit_id":"f8f50397ca1e4ab7f5f31b19dde255ab70b4ccaf"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"273ba6913170137a3cfeb5ca32a9dbf6d6180df8","unresolved":true,"context_lines":[{"line_number":218,"context_line":"            set_resources_quota_usage_dirty(context, resources, tenant_id)"},{"line_number":219,"context_line":"        return 1"},{"line_number":220,"context_line":"    except db_exc.DBError:"},{"line_number":221,"context_line":"        context.session.rollback()"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"@db_api.retry_if_session_inactive()"}],"source_content_type":"text/x-python","patch_set":1,"id":"7770411e_a93624c1","line":221,"range":{"start_line":221,"start_character":8,"end_line":221,"end_character":34},"in_reply_to":"17936f17_901ef0cd","updated":"2021-09-17 13:44:06.000000000","message":"Yeah, that\u0027s why skipping DB error in the middle of transaction is doubtful. IMO we can catch and skip error only if we\u0027re sure method owns transaction, or in other words, if it starts transaction. So we need to find where transaction is started and add both @utils.skip_exceptions(db_exc.DBError) and @transaction_guard there. WDYT? I\u0027d also wait for Rodolfo\u0027s opinion.","commit_id":"f8f50397ca1e4ab7f5f31b19dde255ab70b4ccaf"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"f33cbf1b8ea3641012f47311d337fc6b6f38d262","unresolved":true,"context_lines":[{"line_number":218,"context_line":"            set_resources_quota_usage_dirty(context, resources, tenant_id)"},{"line_number":219,"context_line":"        return 1"},{"line_number":220,"context_line":"    except db_exc.DBError:"},{"line_number":221,"context_line":"        context.session.rollback()"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"@db_api.retry_if_session_inactive()"}],"source_content_type":"text/x-python","patch_set":1,"id":"6219477e_2574a9e8","line":221,"range":{"start_line":221,"start_character":8,"end_line":221,"end_character":34},"in_reply_to":"7770411e_a93624c1","updated":"2021-09-20 09:25:20.000000000","message":"That was my mistake when I changed this method.\n\nThis method \"remove_reservation\" is supposed to create its own DB session, using the context given. This session should be unique for the method execution (decorator in L204). Regardless of the result, any other DB transaction in this context should not be affected.\n\nThe problem I see now is where this method is called [1]: inside an upper session. \"QUOTAS.commit_reservation\" and \"resource_registry.set_resources_dirty\" should be executed in their own sessions (actually \"resource_registry.set_resources_dirty\" is irrelevant with the new DB \"no-lock\" driver)\n\nAs commented in [2], if a reservation is not removed at the end of an API call, it will be removed automatically when testing for expired reservations. In case of having several reservations, each one will be treated independently (this is not usual, most of API calls will create one single reservation).\n\nThe problem is that we use both the v2 API and the Pecan controllers. Both have different paths. The second will call the Pecan hooks, including the quota controller. There we can remove [3] to solve this problem.\n\nIn v2 API, the reservation is removed in [4]. Same as in [3], I recommend to remove this context.\n\nWe also call this method in [5]. What I recommend is to move this call out of this context.\n\nThe last recommendation is to add to this method a transaction guard decorator, to ensure \"remove_reservation\" is always called from outside an active session, forcing it to create its own one.\n\nI\u0027ll push another one with the solution I propose, isolating the reservation removal transactions that is the initial goal of [2].\n\n[1]https://github.com/openstack/neutron/blob/79445f12be3a9ca892672fe0e016336ef60877a2/neutron/pecan_wsgi/hooks/quota_enforcement.py#L80-L85\n[2]https://review.opendev.org/c/openstack/neutron/+/805031\n[3]https://github.com/openstack/neutron/blob/79445f12be3a9ca892672fe0e016336ef60877a2/neutron/pecan_wsgi/hooks/quota_enforcement.py#L80\n[4]https://github.com/openstack/neutron/blob/79445f12be3a9ca892672fe0e016336ef60877a2/neutron/api/v2/base.py#L501-L506\n[5]https://github.com/openstack/neutron/blob/79445f12be3a9ca892672fe0e016336ef60877a2/neutron/db/securitygroups_db.py#L138-L140","commit_id":"f8f50397ca1e4ab7f5f31b19dde255ab70b4ccaf"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"dc743dbac16da30f834f029552da91280744ac76","unresolved":true,"context_lines":[{"line_number":218,"context_line":"            set_resources_quota_usage_dirty(context, resources, tenant_id)"},{"line_number":219,"context_line":"        return 1"},{"line_number":220,"context_line":"    except db_exc.DBError:"},{"line_number":221,"context_line":"        context.session.rollback()"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"@db_api.retry_if_session_inactive()"}],"source_content_type":"text/x-python","patch_set":1,"id":"98e78c24_cb36e54e","line":221,"range":{"start_line":221,"start_character":8,"end_line":221,"end_character":34},"in_reply_to":"8fdaa31a_24668408","updated":"2021-09-17 10:17:00.000000000","message":"by documentation no: https://docs.sqlalchemy.org/en/13/orm/session_api.html?highlight\u003drollback#sqlalchemy.orm.session.Session.rollback","commit_id":"f8f50397ca1e4ab7f5f31b19dde255ab70b4ccaf"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"2e997f93c0f05c5c06453da47cc788f843aac443","unresolved":true,"context_lines":[{"line_number":218,"context_line":"            set_resources_quota_usage_dirty(context, resources, tenant_id)"},{"line_number":219,"context_line":"        return 1"},{"line_number":220,"context_line":"    except db_exc.DBError:"},{"line_number":221,"context_line":"        context.session.rollback()"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"@db_api.retry_if_session_inactive()"}],"source_content_type":"text/x-python","patch_set":1,"id":"12227701_22514ee2","line":221,"range":{"start_line":221,"start_character":8,"end_line":221,"end_character":34},"in_reply_to":"98e78c24_cb36e54e","updated":"2021-09-17 11:05:15.000000000","message":"I mean it can rollback DB changes made before this func, which seems unexpected as we cant know where current transaction started and if rollback here is ok for further processing. Maybe @transaction_guard could be useful here","commit_id":"f8f50397ca1e4ab7f5f31b19dde255ab70b4ccaf"}]}