)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"11de07a5d395bf3604ee2f75b3ee66a421f3925f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"ba119a96_80d96d90","updated":"2022-08-03 23:00:11.000000000","message":"I created this POC patch using the bump_revision() method. It passes the two unit test cases that you created. Let\u0027s see if it passes all the Zuul jobs","commit_id":"4c9cb83d6b46a6425e603194649a61f51a07a307"},{"author":{"_account_id":13861,"name":"yatin","email":"ykarel@redhat.com","username":"yatinkarel"},"change_message_id":"1f4eb9b2b2a9a41044e2eae13863a9383657fa80","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e078300c_ffa090f7","updated":"2022-08-26 11:21:47.000000000","message":"LGTM","commit_id":"4c9cb83d6b46a6425e603194649a61f51a07a307"},{"author":{"_account_id":8313,"name":"Lajos Katona","display_name":"lajoskatona","email":"katonalala@gmail.com","username":"elajkat","status":"Ericsson Software Technology"},"change_message_id":"9097f2a1687c75239e8d8ff00515facba00891d8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"0f09f322_fb209cdd","updated":"2022-08-26 09:31:48.000000000","message":"Looks ok and really bumps rev number","commit_id":"4c9cb83d6b46a6425e603194649a61f51a07a307"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"4ae8b0faea941d519d9633a5e352b64a25821300","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e0da8f63_add1ef92","updated":"2022-08-26 09:10:22.000000000","message":"Tested manually, it works as expected.","commit_id":"4c9cb83d6b46a6425e603194649a61f51a07a307"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"776fd945c9956d38cdb8b97748e251bace4bfe67","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"7561ad48_78f184a5","updated":"2022-08-03 23:00:43.000000000","message":"https://review.opendev.org/c/openstack/neutron/+/852053","commit_id":"4c9cb83d6b46a6425e603194649a61f51a07a307"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"ee73fa1c158344023d914a695ecefc9100e8909e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"0a66f02b_2cc7c188","updated":"2022-08-26 13:27:51.000000000","message":"recheck - grenade job failed due to error while connecting to keystone endpoint from the compute1 node","commit_id":"4c9cb83d6b46a6425e603194649a61f51a07a307"},{"author":{"_account_id":13861,"name":"yatin","email":"ykarel@redhat.com","username":"yatinkarel"},"change_message_id":"af9b3a941c41585bdd34a2d0ecbb8822feca1f43","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"8741f9dd_6780e681","updated":"2022-08-26 12:49:15.000000000","message":"recheck pmlogger error grenade","commit_id":"4c9cb83d6b46a6425e603194649a61f51a07a307"}],"neutron/services/revisions/revision_plugin.py":[{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"de02c2d131f8c653e98bc1fb31efd1db9b75bcf9","unresolved":true,"context_lines":[{"line_number":49,"context_line":"        db_api.sqla_listen(se.Session, \u0027after_rollback\u0027,"},{"line_number":50,"context_line":"                           self._clear_rev_bumped_flags)"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    def _get_objects_to_bump_revision(self, dirty_objects):"},{"line_number":53,"context_line":"        all_std_attr_objects \u003d []"},{"line_number":54,"context_line":"        objects_to_bump_revision \u003d []"},{"line_number":55,"context_line":"        for dirty_object in dirty_objects:"}],"source_content_type":"text/x-python","patch_set":1,"id":"d9e9bed9_92126c96","line":52,"updated":"2022-08-03 00:39:49.000000000","message":"I would like to have a conversation as to whether this approach is too intrusive in the guts of the revisions plugin. Maybe it is also very complicated and there is a simpler and safer approach. I played a little bit with the unit test cases that you added in this patch, while leaving the revision plugin unmodified. Tracing the execution of a subnet description update this is what happens in sequence:\n\n1) The db plugin calls the IPAM to update the subnet object:  https://github.com/openstack/neutron/blob/master/neutron/db/db_base_plugin_v2.py#L968-L969\n\n2) The IPAM calls the OVO update method: https://github.com/openstack/neutron/blob/master/neutron/db/ipam_backend_mixin.py#L219\n\n3) The OVO update method  https://github.com/openstack/neutron/blob/master/neutron/objects/base.py#L939-L943 receives the following as the fields to be updated:\n\n(Pdb) updates\n{\u0027ipv6_ra_mode\u0027: None, \u0027subnetpool_id\u0027: None, \u0027cidr\u0027: AuthenticIPNetwork(\u002710.0.0.0/24\u0027), \u0027ip_version\u0027: 4, \u0027segment_id\u0027: None, \u0027ipv6_address_mode\u0027: None, \u0027description\u0027: \u0027Test Description\u0027}\n\nThe return of obj_db_api.update_object triggers the execution of the bump_revisions method in the revision plugin with the following in session.dirty:\n\n(Pdb) session.dirty\nIdentitySet([\u003cneutron_lib.db.standard_attr.StandardAttribute[object at 7f165a2c41f0] {id\u003d8, resource_type\u003d\u0027subnets\u0027, description\u003d\u0027Test Description\u0027, revision_number\u003d0, created_at\u003ddatetime.datetime(2022, 8, 3, 0, 12, 23), updated_at\u003dNone}\u003e, \u003cneutron.db.models_v2.Subnet[object at 7f165a298d30] {project_id\u003d\u0027bea7b7b5-aeaa-4282-a093-62e91ea569ae\u0027, id\u003d\u002769d0985e-d8f6-4f69-9a7b-e395c49c3b19\u0027, in_use\u003dFalse, name\u003d\u0027\u0027, network_id\u003d\u0027681f8697-4f61-48e0-958b-998ae4aae58d\u0027, segment_id\u003dNone, subnetpool_id\u003dNone, ip_version\u003d4, cidr\u003d\u002710.0.0.0/24\u0027, gateway_ip\u003d\u002710.0.0.1\u0027, enable_dhcp\u003dTrue, ipv6_ra_mode\u003dNone, ipv6_address_mode\u003dNone, standard_attr_id\u003d8}\u003e])\n\nAs you can see, there are two dirty objects, one of type StandardAttribute and the other of type Subnet, which is also of type HasStandardAttributes. I think this is because the code is asking the DB to update fields in the Subnet object, not only the description in StandardAttribute\n\nIn the case of networks, the DB plugin updates directly the DB (no OVO) here: https://github.com/openstack/neutron/blob/master/neutron/db/db_base_plugin_v2.py#L472-L473. The updates are:\n\n(Pdb) (n)\n{\u0027description\u0027: \u0027Test Description\u0027}\n\nWhen the network.update statement finishes, the execution of the bump_revisions method in the revision plugin is triggered with the following in session.dirty:\n\n(Pdb) session.dirty\nIdentitySet([\u003cneutron_lib.db.standard_attr.StandardAttribute[object at 7f3beebdfc70] {id\u003d6, resource_type\u003d\u0027networks\u0027, description\u003d\u0027Test Description\u0027, revision_number\u003d1, created_at\u003ddatetime.datetime(2022, 8, 3, 0, 28, 52), updated_at\u003ddatetime.datetime(2022, 8, 3, 0, 28, 53)}\u003e])\n\nAnd that is why we fail to bump the revision number.\n\nWouldn\u0027t it be easier to include in the network update one of the fields in the network (without really changing it) to force is in session.dirty? And we can do the same for routers. I didn\u0027t have time tonight to test this approach","commit_id":"4c9cb83d6b46a6425e603194649a61f51a07a307"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"6db3a41fce73e63eb775025a5911a3e9f9f3256b","unresolved":true,"context_lines":[{"line_number":49,"context_line":"        db_api.sqla_listen(se.Session, \u0027after_rollback\u0027,"},{"line_number":50,"context_line":"                           self._clear_rev_bumped_flags)"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    def _get_objects_to_bump_revision(self, dirty_objects):"},{"line_number":53,"context_line":"        all_std_attr_objects \u003d []"},{"line_number":54,"context_line":"        objects_to_bump_revision \u003d []"},{"line_number":55,"context_line":"        for dirty_object in dirty_objects:"}],"source_content_type":"text/x-python","patch_set":1,"id":"c2064279_de95cd49","line":52,"in_reply_to":"d6f37069_1732f9d8","updated":"2022-08-22 11:06:54.000000000","message":"The only reason why I would like to have it fixed in the revision_plugin directly it that it would be done for all resources which have description field from the StandardAttributes class.\nOtherwise we will need to remember to call bump_revision() for every such resource which is IMHO error prone.","commit_id":"4c9cb83d6b46a6425e603194649a61f51a07a307"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"105396fb9491ffc7f02d9120676bc8709cd6e350","unresolved":true,"context_lines":[{"line_number":49,"context_line":"        db_api.sqla_listen(se.Session, \u0027after_rollback\u0027,"},{"line_number":50,"context_line":"                           self._clear_rev_bumped_flags)"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"    def _get_objects_to_bump_revision(self, dirty_objects):"},{"line_number":53,"context_line":"        all_std_attr_objects \u003d []"},{"line_number":54,"context_line":"        objects_to_bump_revision \u003d []"},{"line_number":55,"context_line":"        for dirty_object in dirty_objects:"}],"source_content_type":"text/x-python","patch_set":1,"id":"d6f37069_1732f9d8","line":52,"in_reply_to":"d9e9bed9_92126c96","updated":"2022-08-03 02:50:20.000000000","message":"In fact, why not use the bump_revision() method:\n\nhttps://github.com/openstack/neutron-lib/blob/master/neutron_lib/db/standard_attr.py#L200\n\nOne example is here:\n\nhttps://github.com/openstack/neutron/blob/master/neutron/db/l3_db.py#L610","commit_id":"4c9cb83d6b46a6425e603194649a61f51a07a307"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"4ae8b0faea941d519d9633a5e352b64a25821300","unresolved":true,"context_lines":[{"line_number":70,"context_line":"        # field modified, should have revision bumped too"},{"line_number":71,"context_line":"        objects_to_bump_revision +\u003d ["},{"line_number":72,"context_line":"            o for o in all_std_attr_objects"},{"line_number":73,"context_line":"            if (\u0027description\u0027 not in o._sa_instance_state.unmodified and"},{"line_number":74,"context_line":"                o.id not in std_attr_ids)]"},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"        return objects_to_bump_revision"}],"source_content_type":"text/x-python","patch_set":1,"id":"65c8786d_8f083e44","line":73,"range":{"start_line":73,"start_character":39,"end_line":73,"end_character":57},"updated":"2022-08-26 09:10:22.000000000","message":"nit: I think we can retrieve that using a public method: sqlalmechy.orm.base.instance_state. I tried this:\n\nfrom sqlalchemy.orm import base as orm_base\na \u003d orm_base.instance_state(all_std_attr_objects[0])\n# a.unmodified is the required set","commit_id":"4c9cb83d6b46a6425e603194649a61f51a07a307"}]}