)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"c86202af067ce1457f3ab67c45a3a72faf94e34d","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"While setting the event handler lock in OvnNbIdlForLb"},{"line_number":10,"context_line":"we found that on each Octavia API call exception"},{"line_number":11,"context_line":"was raised that the IDL has not aquired OVNDB lock [0]."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"It is because when we set the event lock name it is"},{"line_number":14,"context_line":"required to be aquired on each transaction, even on"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"7faddb67_ccbfdccd","line":11,"range":{"start_line":11,"start_character":32,"end_line":11,"end_character":39},"updated":"2019-08-21 10:59:05.000000000","message":"acquired","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"4d28ad5b0e9203c66cc4b97f873df9820cc7cb32","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"While setting the event handler lock in OvnNbIdlForLb"},{"line_number":10,"context_line":"we found that on each Octavia API call exception"},{"line_number":11,"context_line":"was raised that the IDL has not aquired OVNDB lock [0]."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"It is because when we set the event lock name it is"},{"line_number":14,"context_line":"required to be aquired on each transaction, even on"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"7faddb67_786c62cf","line":11,"range":{"start_line":11,"start_character":32,"end_line":11,"end_character":39},"in_reply_to":"7faddb67_ccbfdccd","updated":"2019-08-21 14:42:53.000000000","message":"++","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"}],"networking_ovn/octavia/ovn_driver.py":[{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"142e77a7e453238a4d1fd5320a3146b93f2b769b","unresolved":false,"context_lines":[{"line_number":163,"context_line":"    def start(self):"},{"line_number":164,"context_line":"        self.conn \u003d connection.Connection("},{"line_number":165,"context_line":"            self, timeout\u003dovn_cfg.get_ovn_ovsdb_timeout())"},{"line_number":166,"context_line":"        # NOTE(mjozecz): Do not reuse connection in OvnNbApiIdlImpl."},{"line_number":167,"context_line":"        # OvnNbIdlForLb object would be initialized only once, so no worries."},{"line_number":168,"context_line":"        idl_ovn.OvnNbApiIdlImpl.ovsdb_connection \u003d None"},{"line_number":169,"context_line":"        return idl_ovn.OvnNbApiIdlImpl(self.conn)"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    def stop(self):"},{"line_number":172,"context_line":"        # Close the running connection"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_5ad2fe5d","line":169,"range":{"start_line":166,"start_character":0,"end_line":169,"end_character":49},"updated":"2019-08-01 09:44:10.000000000","message":"My setting it to None, are we closing that connection properly ?\n\nAlso, this seems something that we can later fix in ovsdbapp","commit_id":"8045b175cc5b1daeb3341bc75e243ba58ba23ead"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"9d1aa4e2ba13a55017dc2b293f338d435efed7ac","unresolved":false,"context_lines":[{"line_number":163,"context_line":"    def start(self):"},{"line_number":164,"context_line":"        self.conn \u003d connection.Connection("},{"line_number":165,"context_line":"            self, timeout\u003dovn_cfg.get_ovn_ovsdb_timeout())"},{"line_number":166,"context_line":"        # NOTE(mjozecz): Do not reuse connection in OvnNbApiIdlImpl."},{"line_number":167,"context_line":"        # OvnNbIdlForLb object would be initialized only once, so no worries."},{"line_number":168,"context_line":"        idl_ovn.OvnNbApiIdlImpl.ovsdb_connection \u003d None"},{"line_number":169,"context_line":"        return idl_ovn.OvnNbApiIdlImpl(self.conn)"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"    def stop(self):"},{"line_number":172,"context_line":"        # Close the running connection"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_754d8b70","line":169,"range":{"start_line":166,"start_character":0,"end_line":169,"end_character":49},"in_reply_to":"7faddb67_5ad2fe5d","updated":"2019-08-01 10:36:26.000000000","message":"Looks like the stop() methods are not executed at destroying the IDL\u0027s. I\u0027ll use atexit to be sure. Thanks for pointing this! :D","commit_id":"8045b175cc5b1daeb3341bc75e243ba58ba23ead"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"142e77a7e453238a4d1fd5320a3146b93f2b769b","unresolved":false,"context_lines":[{"line_number":265,"context_line":"                self.ovn_nb_idl_for_o_api.start())"},{"line_number":266,"context_line":"        else:"},{"line_number":267,"context_line":"            self.ovn_nb_idl_for_o_api \u003d ("},{"line_number":268,"context_line":"                OvnProviderHelper.ovn_nbdb_api_for_o_api)"},{"line_number":269,"context_line":""},{"line_number":270,"context_line":"        self.helper_thread.start()"},{"line_number":271,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_7aa6dac3","line":268,"updated":"2019-08-01 09:44:10.000000000","message":"Maybe you already thought about it, but, in theory since we are instantiating both IDLs only once by using a class attribute we could have the same IDL handling both API requests and events, no ? (Maybe it\u0027s slower idk)\n\n/me is not against the current design, just curious","commit_id":"8045b175cc5b1daeb3341bc75e243ba58ba23ead"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"58432c2448fa7884b9ed03ed5316c83370446df7","unresolved":false,"context_lines":[{"line_number":265,"context_line":"                self.ovn_nb_idl_for_o_api.start())"},{"line_number":266,"context_line":"        else:"},{"line_number":267,"context_line":"            self.ovn_nb_idl_for_o_api \u003d ("},{"line_number":268,"context_line":"                OvnProviderHelper.ovn_nbdb_api_for_o_api)"},{"line_number":269,"context_line":""},{"line_number":270,"context_line":"        self.helper_thread.start()"},{"line_number":271,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_da6cae37","line":268,"in_reply_to":"7faddb67_7aa6dac3","updated":"2019-08-01 10:01:43.000000000","message":"I forgot that for the event IDL we need to set a lock, my bad!\n\nThanks for clarifying on IRC maciej!","commit_id":"8045b175cc5b1daeb3341bc75e243ba58ba23ead"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"f587477418e0667e1529a2f5ae509a16be982c7c","unresolved":false,"context_lines":[{"line_number":265,"context_line":"                self.ovn_nb_idl_for_o_api.start())"},{"line_number":266,"context_line":"        else:"},{"line_number":267,"context_line":"            self.ovn_nb_idl_for_o_api \u003d ("},{"line_number":268,"context_line":"                OvnProviderHelper.ovn_nbdb_api_for_o_api)"},{"line_number":269,"context_line":""},{"line_number":270,"context_line":"        self.helper_thread.start()"},{"line_number":271,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_f7003f38","line":268,"in_reply_to":"7faddb67_da6cae37","updated":"2019-08-07 19:18:54.000000000","message":"I\u0027m still not sure I understand the issue with events/locks. We create a single shared IDL, it grabs the lock, and we watch events for the driver at each start(). Then at shutdown() we stop watching events for that API call\u0027s driver.\n\nI\u0027m not that familiar with the actual architecture of the octavia stuff, though. Is the reason we have the lock just because we *used* to do multiple IDLs and wanted to make sure only one handled events? Do we need the lock anymore since we only have one IDL?","commit_id":"8045b175cc5b1daeb3341bc75e243ba58ba23ead"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"31da852d1f44c62a6a599da756891c39e5c8388f","unresolved":false,"context_lines":[{"line_number":265,"context_line":"                self.ovn_nb_idl_for_o_api.start())"},{"line_number":266,"context_line":"        else:"},{"line_number":267,"context_line":"            self.ovn_nb_idl_for_o_api \u003d ("},{"line_number":268,"context_line":"                OvnProviderHelper.ovn_nbdb_api_for_o_api)"},{"line_number":269,"context_line":""},{"line_number":270,"context_line":"        self.helper_thread.start()"},{"line_number":271,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_9e431c14","line":268,"in_reply_to":"7faddb67_f7003f38","updated":"2019-08-08 09:17:57.000000000","message":"Octavia OVN provider driver is responsible for 2 things:\n1) Handling the octavia API requests and apply changes on ovn-nb, like: add member to pool\n2) Handling incoming OVSDB events related to LRP and LS - for example applying the given LB on LS of network that has been attached to LR, in order to reach LB members from LS network via router.\n\nUnfortunately while setting the lock_name for handling events (responsibility 2), the IDL is configured to need the lock all the time. While using the same IDL for handling Octavia API requests (responsibility 2), the OVSDB transaction fails each time on: \"The transaction failed because the IDL has been configured to require a database lock but didn\u0027t get it yet or has already lost it\" [0]. Thats the ovsdb thing.\n\nSo my goal here is to initialize 2 IDLs with 2 active connections to OVN NB DB. One will be configured with lock_name (to prevent two octavia running workers to handle the same events), and the second will be configured without lock_name - to handle API requests. [1]\n\nIn next months we\u0027ll have possibility to spawn long-running thread in Octavia, just only to handle events (responsibility 2). In this term potentially we could drop this commit from master. Unfortunately we\u0027ll still need this in Stein, because the long-running thread will not be cherry-picked to Stein (from Octavia side).\n\nWe need to have lock set for events, because other Octavia API instance (yes Octavia could scale) will be handling the same events in the same time.\n\n[0] https://github.com/openstack/ovsdbapp/blob/master/ovsdbapp/backend/ovs_idl/transaction.py#L102\n[1] https://github.com/openvswitch/ovs/blob/master/python/ovs/db/idl.py#L445","commit_id":"8045b175cc5b1daeb3341bc75e243ba58ba23ead"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"4ac9470828d1c6c0c74290f9cd469c056074b6ff","unresolved":false,"context_lines":[{"line_number":173,"context_line":"        # Close the running connection"},{"line_number":174,"context_line":"        if not self.conn.stop(timeout\u003dovn_cfg.get_ovn_ovsdb_timeout()):"},{"line_number":175,"context_line":"            LOG.debug(\"Connection terminated to OvnNb \""},{"line_number":176,"context_line":"                      \"but a thread is still alive\")"},{"line_number":177,"context_line":"        # Close the idl session"},{"line_number":178,"context_line":"        self.close()"},{"line_number":179,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"7faddb67_887ede67","line":176,"range":{"start_line":176,"start_character":22,"end_line":176,"end_character":51},"updated":"2019-08-19 12:14:08.000000000","message":"Will the thread eventually resume ?","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"c86202af067ce1457f3ab67c45a3a72faf94e34d","unresolved":false,"context_lines":[{"line_number":259,"context_line":"            OvnProviderHelper.ovn_nbdb_api_for_o_api \u003d ("},{"line_number":260,"context_line":"                OvnProviderHelper.ovn_nb_idl_for_o_api.start())"},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"        self.ovn_nbdb_api_for_o_api \u003d OvnProviderHelper.ovn_nbdb_api_for_o_api"},{"line_number":263,"context_line":"        self.ovn_nb_idl_for_o_api \u003d OvnProviderHelper.ovn_nb_idl_for_o_api"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"        # NOTE(mjozefcz): This IDL is only for handling OVSDB events!"}],"source_content_type":"text/x-python","patch_set":5,"id":"7faddb67_ac8f0021","line":262,"updated":"2019-08-21 10:59:05.000000000","message":"why is this needed? if MRO doesn\u0027t find an attribute on instance level, it searches in class, doesn\u0027t it?","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"4d28ad5b0e9203c66cc4b97f873df9820cc7cb32","unresolved":false,"context_lines":[{"line_number":259,"context_line":"            OvnProviderHelper.ovn_nbdb_api_for_o_api \u003d ("},{"line_number":260,"context_line":"                OvnProviderHelper.ovn_nb_idl_for_o_api.start())"},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"        self.ovn_nbdb_api_for_o_api \u003d OvnProviderHelper.ovn_nbdb_api_for_o_api"},{"line_number":263,"context_line":"        self.ovn_nb_idl_for_o_api \u003d OvnProviderHelper.ovn_nb_idl_for_o_api"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"        # NOTE(mjozefcz): This IDL is only for handling OVSDB events!"}],"source_content_type":"text/x-python","patch_set":5,"id":"7faddb67_33d04b1f","line":262,"in_reply_to":"7faddb67_ac8f0021","updated":"2019-08-21 14:42:53.000000000","message":"Yes, you\u0027re totally right :), Thanks :)","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"4ac9470828d1c6c0c74290f9cd469c056074b6ff","unresolved":false,"context_lines":[{"line_number":274,"context_line":"        self.ovn_nb_idl \u003d OvnProviderHelper.ovn_nb_idl"},{"line_number":275,"context_line":""},{"line_number":276,"context_line":"        # NOTE(mjozefcz): Because of this property:"},{"line_number":277,"context_line":"        # https://github.com/openstack/ovsdbapp/blob/master/ovsdbapp/backend/ovs_idl/__init__.py#L51"},{"line_number":278,"context_line":"        # we want to set now \u0027default\u0027 ovsdb_connection class attribute to be"},{"line_number":279,"context_line":"        # the one for API calls."},{"line_number":280,"context_line":"        idl_ovn.OvnNbApiIdlImpl.ovsdb_connection \u003d ("}],"source_content_type":"text/x-python","patch_set":5,"id":"7faddb67_48d9c680","line":277,"range":{"start_line":277,"start_character":10,"end_line":277,"end_character":100},"updated":"2019-08-19 12:14:08.000000000","message":"Tip: If you press \"y\" in the github link it will give you a permanent link to that page/reference. That way, if the file changes the comment will still be pointing to the right place","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"4d28ad5b0e9203c66cc4b97f873df9820cc7cb32","unresolved":false,"context_lines":[{"line_number":274,"context_line":"        self.ovn_nb_idl \u003d OvnProviderHelper.ovn_nb_idl"},{"line_number":275,"context_line":""},{"line_number":276,"context_line":"        # NOTE(mjozefcz): Because of this property:"},{"line_number":277,"context_line":"        # https://github.com/openstack/ovsdbapp/blob/master/ovsdbapp/backend/ovs_idl/__init__.py#L51"},{"line_number":278,"context_line":"        # we want to set now \u0027default\u0027 ovsdb_connection class attribute to be"},{"line_number":279,"context_line":"        # the one for API calls."},{"line_number":280,"context_line":"        idl_ovn.OvnNbApiIdlImpl.ovsdb_connection \u003d ("}],"source_content_type":"text/x-python","patch_set":5,"id":"7faddb67_6cc828a7","line":277,"range":{"start_line":277,"start_character":10,"end_line":277,"end_character":100},"in_reply_to":"7faddb67_48d9c680","updated":"2019-08-21 14:42:53.000000000","message":"++","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"c86202af067ce1457f3ab67c45a3a72faf94e34d","unresolved":false,"context_lines":[{"line_number":285,"context_line":"    def shutdown(self):"},{"line_number":286,"context_line":"        self.requests.put({\u0027type\u0027: REQ_TYPE_EXIT})"},{"line_number":287,"context_line":"        self.helper_thread.join()"},{"line_number":288,"context_line":"        if hasattr(self.ovn_nb_idl, \u0027notify_handler\u0027):"},{"line_number":289,"context_line":"            self.ovn_nb_idl.notify_handler.unwatch_events(self.events)"},{"line_number":290,"context_line":""},{"line_number":291,"context_line":"    @staticmethod"}],"source_content_type":"text/x-python","patch_set":5,"id":"7faddb67_2cfa3074","line":288,"updated":"2019-08-21 10:59:05.000000000","message":"When is this False? notify_handler is defined in __init__","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"4d28ad5b0e9203c66cc4b97f873df9820cc7cb32","unresolved":false,"context_lines":[{"line_number":285,"context_line":"    def shutdown(self):"},{"line_number":286,"context_line":"        self.requests.put({\u0027type\u0027: REQ_TYPE_EXIT})"},{"line_number":287,"context_line":"        self.helper_thread.join()"},{"line_number":288,"context_line":"        if hasattr(self.ovn_nb_idl, \u0027notify_handler\u0027):"},{"line_number":289,"context_line":"            self.ovn_nb_idl.notify_handler.unwatch_events(self.events)"},{"line_number":290,"context_line":""},{"line_number":291,"context_line":"    @staticmethod"}],"source_content_type":"text/x-python","patch_set":5,"id":"7faddb67_d3d53744","line":288,"in_reply_to":"7faddb67_2cfa3074","updated":"2019-08-21 14:42:53.000000000","message":"I got some failures during work on this patch but yeah it could be removed now. Thanks for pointing.","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"c86202af067ce1457f3ab67c45a3a72faf94e34d","unresolved":false,"context_lines":[{"line_number":483,"context_line":"        return self.ovn_nbdb_api_for_o_api.lookup(\u0027Load_Balancer\u0027, lb_id)"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"    def _find_ovn_lb_with_pool_key(self, pool_key):"},{"line_number":486,"context_line":"        lbs \u003d self.ovn_nbdb_api_for_o_api.db_list_rows(\u0027Load_Balancer\u0027)"},{"line_number":487,"context_line":"        for lb in lbs.execute():"},{"line_number":488,"context_line":"            if pool_key in lb.external_ids:"},{"line_number":489,"context_line":"                return lb"},{"line_number":490,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"7faddb67_cc7cbcec","line":487,"range":{"start_line":486,"start_character":0,"end_line":487,"end_character":32},"updated":"2019-08-21 10:59:05.000000000","message":"supernit: I\u0027d rather do\n\n lbs \u003d self.ovn_nbdb_api_for_o_api.db_list_rows(\n     \u0027Load_Balancer\u0027).execute()","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"4d28ad5b0e9203c66cc4b97f873df9820cc7cb32","unresolved":false,"context_lines":[{"line_number":483,"context_line":"        return self.ovn_nbdb_api_for_o_api.lookup(\u0027Load_Balancer\u0027, lb_id)"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"    def _find_ovn_lb_with_pool_key(self, pool_key):"},{"line_number":486,"context_line":"        lbs \u003d self.ovn_nbdb_api_for_o_api.db_list_rows(\u0027Load_Balancer\u0027)"},{"line_number":487,"context_line":"        for lb in lbs.execute():"},{"line_number":488,"context_line":"            if pool_key in lb.external_ids:"},{"line_number":489,"context_line":"                return lb"},{"line_number":490,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"7faddb67_9834be4a","line":487,"range":{"start_line":486,"start_character":0,"end_line":487,"end_character":32},"in_reply_to":"7faddb67_cc7cbcec","updated":"2019-08-21 14:42:53.000000000","message":"done, I\u0027ve also added check_error\u003dTrue","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"ac26f971b3d12018857e31a228a68beedf1c366c","unresolved":false,"context_lines":[{"line_number":483,"context_line":"        return self.ovn_nbdb_api_for_o_api.lookup(\u0027Load_Balancer\u0027, lb_id)"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"    def _find_ovn_lb_with_pool_key(self, pool_key):"},{"line_number":486,"context_line":"        lbs \u003d self.ovn_nbdb_api_for_o_api.db_list_rows(\u0027Load_Balancer\u0027)"},{"line_number":487,"context_line":"        for lb in lbs.execute():"},{"line_number":488,"context_line":"            if pool_key in lb.external_ids:"},{"line_number":489,"context_line":"                return lb"},{"line_number":490,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"7faddb67_471a57c6","line":487,"range":{"start_line":486,"start_character":0,"end_line":487,"end_character":32},"in_reply_to":"7faddb67_cc7cbcec","updated":"2019-08-29 13:54:53.000000000","message":"great catch!","commit_id":"6ae4b9ec3bbea9d11c6b1da0744d41c4c8fc3dd0"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"6b2a90ce5ca6136ce20894f030ea97ad27f97a7b","unresolved":false,"context_lines":[{"line_number":29,"context_line":"from ovsdbapp.backend.ovs_idl import connection"},{"line_number":30,"context_line":"from ovsdbapp.backend.ovs_idl import event as row_event"},{"line_number":31,"context_line":"from ovsdbapp.backend.ovs_idl import idlutils"},{"line_number":32,"context_line":"from ovsdbapp.schema.ovn_northbound import impl_idl as idl_ovn"},{"line_number":33,"context_line":"from six.moves import queue as Queue"},{"line_number":34,"context_line":"from stevedore import driver"},{"line_number":35,"context_line":"import tenacity"}],"source_content_type":"text/x-python","patch_set":6,"id":"7faddb67_eeb69325","line":32,"updated":"2019-09-03 07:42:50.000000000","message":"That is why the connection is not being reused.","commit_id":"ba2c4a9665e3fd88498b94dc6064eaa1618b4e54"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"00a1f2fb33df750ba01dbf4df8db15504197568a","unresolved":false,"context_lines":[{"line_number":142,"context_line":"class OvnNbIdlForLb(ovsdb_monitor.OvnIdl):"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"    SCHEMA \u003d \"OVN_Northbound\""},{"line_number":145,"context_line":"    TABLES \u003d (\u0027Logical_Switch\u0027, \u0027Load_Balancer\u0027, \u0027Logical_Router\u0027,"},{"line_number":146,"context_line":"              \u0027Logical_Switch_Port\u0027, \u0027Logical_Router_Port\u0027,"},{"line_number":147,"context_line":"              \u0027Gateway_Chassis\u0027)"},{"line_number":148,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"7faddb67_f6f0fadc","line":145,"updated":"2019-08-29 16:33:13.000000000","message":"nit: While I absolutely approve of this change, it doesn\u0027t look specifically related to this patch. So if we ever ended up having to revert this change for some reason, we\u0027d also lose this harmless unrelated improvement as well.","commit_id":"ba2c4a9665e3fd88498b94dc6064eaa1618b4e54"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"6b2a90ce5ca6136ce20894f030ea97ad27f97a7b","unresolved":false,"context_lines":[{"line_number":142,"context_line":"class OvnNbIdlForLb(ovsdb_monitor.OvnIdl):"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"    SCHEMA \u003d \"OVN_Northbound\""},{"line_number":145,"context_line":"    TABLES \u003d (\u0027Logical_Switch\u0027, \u0027Load_Balancer\u0027, \u0027Logical_Router\u0027,"},{"line_number":146,"context_line":"              \u0027Logical_Switch_Port\u0027, \u0027Logical_Router_Port\u0027,"},{"line_number":147,"context_line":"              \u0027Gateway_Chassis\u0027)"},{"line_number":148,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"7faddb67_5db523ec","line":145,"in_reply_to":"7faddb67_f6f0fadc","updated":"2019-09-03 07:42:50.000000000","message":"We\u0027ll not revert the code. Based on your comment I found that we import in L32:\n\nfrom ovsdbapp.schema.ovn_northbound import impl_idl as idl_ovn\n\nnot:\nfrom networking_ovn.ovsdb import impl_idl_ovn\n\nThat makes the difference. The second one utilize modified networking_ovn.ovsdb.impl_idl_ovn.Backend inherited by OvsdbNbOvnIdl as per your comment. So after changing that there is no need to set None for connection in L274.\nThanks a lot for giving me this clue!!!\n\nSo base on that fact I\u0027ll leave it as it is.","commit_id":"ba2c4a9665e3fd88498b94dc6064eaa1618b4e54"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"00a1f2fb33df750ba01dbf4df8db15504197568a","unresolved":false,"context_lines":[{"line_number":166,"context_line":"            self, timeout\u003dovn_cfg.get_ovn_ovsdb_timeout())"},{"line_number":167,"context_line":"        # NOTE(mjozecz): Do not reuse connection in OvnNbApiIdlImpl."},{"line_number":168,"context_line":"        # OvnNbIdlForLb object would be initialized only once, so no worries."},{"line_number":169,"context_line":"        idl_ovn.OvnNbApiIdlImpl.ovsdb_connection \u003d None"},{"line_number":170,"context_line":"        return idl_ovn.OvnNbApiIdlImpl(self.conn)"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def stop(self):"}],"source_content_type":"text/x-python","patch_set":6,"id":"7faddb67_91a8b020","line":169,"updated":"2019-08-29 16:33:13.000000000","message":"I\u0027m having trouble understanding this line. ovsdb_connection is an instance variable in OvnNbApiIdlImpl, so setting the class variable shouldn\u0027t have any effect except for any code in networking-ovn that directly uses the class variable (that is still lurking due to existing in ovsdbapp.Backend).\n\nIt seems like there should always be an instance ovsdb_connection set, so I don\u0027t yet understand why line 274 below also specifically sets the class ovsdb_connection. Shouldn\u0027t each individual instance already be getting its own connection due to the overridden networking_ovn.ovsdb.impl_idl_ovn.Backend inherited by OvsdbNbOvnIdl, and therefor just pass in the connection it wants to use when being initialized?\n\nAlso, I apologize for ever using a Backend mixin, enforcing the scope of ovsdb_connection in the ovsdbapp api, and starting a db connection in __init__() by default. This is far more confusing than it should be. :p I can only plead that we were replacing the ovs-vsctl-based implementation in Neutron and so it didn\u0027t have the concept of a connection and I wanted it to be a drop-in replacement. I should have made that small part of the code live in neutron itself.","commit_id":"ba2c4a9665e3fd88498b94dc6064eaa1618b4e54"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"6b2a90ce5ca6136ce20894f030ea97ad27f97a7b","unresolved":false,"context_lines":[{"line_number":166,"context_line":"            self, timeout\u003dovn_cfg.get_ovn_ovsdb_timeout())"},{"line_number":167,"context_line":"        # NOTE(mjozecz): Do not reuse connection in OvnNbApiIdlImpl."},{"line_number":168,"context_line":"        # OvnNbIdlForLb object would be initialized only once, so no worries."},{"line_number":169,"context_line":"        idl_ovn.OvnNbApiIdlImpl.ovsdb_connection \u003d None"},{"line_number":170,"context_line":"        return idl_ovn.OvnNbApiIdlImpl(self.conn)"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"    def stop(self):"}],"source_content_type":"text/x-python","patch_set":6,"id":"7faddb67_6eb70387","line":169,"in_reply_to":"7faddb67_91a8b020","updated":"2019-09-03 07:42:50.000000000","message":"That is really useful. I didn\u0027t know that we override Backend implementation in networking-ovn. That makes whole things easier :) Thanks!!! Please see next PS5. That is more clear.\n\nThanks a lot!","commit_id":"ba2c4a9665e3fd88498b94dc6064eaa1618b4e54"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"00a1f2fb33df750ba01dbf4df8db15504197568a","unresolved":false,"context_lines":[{"line_number":254,"context_line":""},{"line_number":255,"context_line":"    def start(self):"},{"line_number":256,"context_line":"        # NOTE(mjozefcz): This IDL is only for handling octavia API requests."},{"line_number":257,"context_line":"        if not OvnProviderHelper.ovn_nbdb_api_for_o_api:"},{"line_number":258,"context_line":"            OvnProviderHelper.ovn_nb_idl_for_o_api \u003d OvnNbIdlForLb()"},{"line_number":259,"context_line":"            OvnProviderHelper.ovn_nbdb_api_for_o_api \u003d ("},{"line_number":260,"context_line":"                OvnProviderHelper.ovn_nb_idl_for_o_api.start())"}],"source_content_type":"text/x-python","patch_set":6,"id":"7faddb67_a7ceab2a","line":257,"updated":"2019-08-29 16:33:13.000000000","message":"nit: The patch would be a lot smaller if the thing that changed names was the api for handling events. Changing all of the ovn_nbdb_api to nbdb_api_for_o_api throughout the rest of the patch and breaking up lines, etc. adds a lot of lines.","commit_id":"ba2c4a9665e3fd88498b94dc6064eaa1618b4e54"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"6b2a90ce5ca6136ce20894f030ea97ad27f97a7b","unresolved":false,"context_lines":[{"line_number":254,"context_line":""},{"line_number":255,"context_line":"    def start(self):"},{"line_number":256,"context_line":"        # NOTE(mjozefcz): This IDL is only for handling octavia API requests."},{"line_number":257,"context_line":"        if not OvnProviderHelper.ovn_nbdb_api_for_o_api:"},{"line_number":258,"context_line":"            OvnProviderHelper.ovn_nb_idl_for_o_api \u003d OvnNbIdlForLb()"},{"line_number":259,"context_line":"            OvnProviderHelper.ovn_nbdb_api_for_o_api \u003d ("},{"line_number":260,"context_line":"                OvnProviderHelper.ovn_nb_idl_for_o_api.start())"}],"source_content_type":"text/x-python","patch_set":6,"id":"7faddb67_ee7773d0","line":257,"in_reply_to":"7faddb67_a7ceab2a","updated":"2019-09-03 07:42:50.000000000","message":"Yep, I named the IDL that handles events to be ovn_ndbd_api. With other names it didn\u0027t work (idk if the ovn_nbdb_api was kind of registered attr name...). But with the PS5 patch it suddenly started. So I moved the name back. Thanks!","commit_id":"ba2c4a9665e3fd88498b94dc6064eaa1618b4e54"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"00a1f2fb33df750ba01dbf4df8db15504197568a","unresolved":false,"context_lines":[{"line_number":271,"context_line":"        # https://github.com/openstack/ovsdbapp/blob/68d462d355a9c9f7e6b12f08f4dfb0b0b3d443b2/ovsdbapp/backend/ovs_idl/__init__.py#L51"},{"line_number":272,"context_line":"        # we want to set now \u0027default\u0027 ovsdb_connection class attribute to be"},{"line_number":273,"context_line":"        # the one for API calls."},{"line_number":274,"context_line":"        idl_ovn.OvnNbApiIdlImpl.ovsdb_connection \u003d ("},{"line_number":275,"context_line":"            OvnProviderHelper.ovn_nb_idl_for_o_api.conn)"},{"line_number":276,"context_line":""},{"line_number":277,"context_line":"        self.helper_thread.start()"}],"source_content_type":"text/x-python","patch_set":6,"id":"7faddb67_3bb66fde","line":274,"updated":"2019-08-29 16:33:13.000000000","message":"see note at line 169","commit_id":"ba2c4a9665e3fd88498b94dc6064eaa1618b4e54"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"113df2633db2a0f6a03c511016edda1925c87c85","unresolved":false,"context_lines":[{"line_number":240,"context_line":"    def start(self):"},{"line_number":241,"context_line":"        # NOTE(mjozefcz): This IDL is only for handling octavia API requests."},{"line_number":242,"context_line":"        if not OvnProviderHelper.ovn_nbdb_api:"},{"line_number":243,"context_line":"            OvnProviderHelper.ovn_nb_idl \u003d OvnNbIdlForLb()"},{"line_number":244,"context_line":"            OvnProviderHelper.ovn_nbdb_api \u003d ("},{"line_number":245,"context_line":"                OvnProviderHelper.ovn_nb_idl.start())"},{"line_number":246,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"7faddb67_dd17bda2","line":243,"updated":"2019-09-04 14:59:52.000000000","message":"nit: no reason to store this on the class instead of just doing:\n\n    OvnProviderHelper.ovn_nbdb_api \u003d OvnNbIdlForLb().start()\n\nespecially since \u0027ovn_nb_idl\u0027 is never directly used outside of this method.","commit_id":"b0fabe3bcf037bf186c5d18925d2b4c9166c444f"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"e5f42656ab3e4b4ac9d232d479c8b1ec0af842d8","unresolved":false,"context_lines":[{"line_number":240,"context_line":"    def start(self):"},{"line_number":241,"context_line":"        # NOTE(mjozefcz): This IDL is only for handling octavia API requests."},{"line_number":242,"context_line":"        if not OvnProviderHelper.ovn_nbdb_api:"},{"line_number":243,"context_line":"            OvnProviderHelper.ovn_nb_idl \u003d OvnNbIdlForLb()"},{"line_number":244,"context_line":"            OvnProviderHelper.ovn_nbdb_api \u003d ("},{"line_number":245,"context_line":"                OvnProviderHelper.ovn_nb_idl.start())"},{"line_number":246,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"7faddb67_82e216d3","line":243,"in_reply_to":"7faddb67_dd17bda2","updated":"2019-09-05 10:28:13.000000000","message":"Done","commit_id":"b0fabe3bcf037bf186c5d18925d2b4c9166c444f"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"eecacd980bb034aab4367cafc98bf64738f92cfa","unresolved":false,"context_lines":[{"line_number":186,"context_line":"    ovn_nbdb_api_for_events \u003d None"},{"line_number":187,"context_line":"    ovn_nb_idl_for_events \u003d None"},{"line_number":188,"context_line":"    ovn_nbdb_api \u003d None"},{"line_number":189,"context_line":"    ovn_nb_idl \u003d None"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    def __init__(self):"},{"line_number":192,"context_line":"        self.requests \u003d Queue.Queue()"}],"source_content_type":"text/x-python","patch_set":13,"id":"5faad753_5baa5ed2","line":189,"range":{"start_line":189,"start_character":4,"end_line":189,"end_character":21},"updated":"2019-09-10 14:46:06.000000000","message":"unused","commit_id":"bbc372917b0e177a0bc840be8eb3878592055d71"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"dbee6e95442e6a024c0793e7f6d4b04802cbb14c","unresolved":false,"context_lines":[{"line_number":186,"context_line":"    ovn_nbdb_api_for_events \u003d None"},{"line_number":187,"context_line":"    ovn_nb_idl_for_events \u003d None"},{"line_number":188,"context_line":"    ovn_nbdb_api \u003d None"},{"line_number":189,"context_line":"    ovn_nb_idl \u003d None"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    def __init__(self):"},{"line_number":192,"context_line":"        self.requests \u003d Queue.Queue()"}],"source_content_type":"text/x-python","patch_set":13,"id":"5faad753_a9f3fa9d","line":189,"range":{"start_line":189,"start_character":4,"end_line":189,"end_character":21},"in_reply_to":"5faad753_5baa5ed2","updated":"2019-09-11 07:00:27.000000000","message":"Eagle eye :)","commit_id":"bbc372917b0e177a0bc840be8eb3878592055d71"}],"networking_ovn/tests/unit/octavia/test_ovn_driver.py":[{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"d6dffec06c7f5e86409068604e0cea5ff3a995ad","unresolved":false,"context_lines":[{"line_number":11,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":12,"context_line":"#    under the License."},{"line_number":13,"context_line":"#"},{"line_number":14,"context_line":"import os"},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"import mock"},{"line_number":17,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"5faad753_941df6be","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":9},"updated":"2019-09-11 08:37:01.000000000","message":"nit: this is adding a new import block. Maybe unrelated to the patch but we should have:\n\nimport os\n\nimport mock\nfrom neutron.tests import base\n.....\n\n\nhttps://docs.openstack.org/hacking/latest/user/hacking.html\n\n{{stdlib imports in human alphabetical order}}\n\\n\n{{third-party lib imports in human alphabetical order}}\n\\n\n{{project imports in human alphabetical order}}","commit_id":"bbc372917b0e177a0bc840be8eb3878592055d71"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"093080727b6d867cbe2c3f35b9f4d1d58d63e387","unresolved":false,"context_lines":[{"line_number":11,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":12,"context_line":"#    under the License."},{"line_number":13,"context_line":"#"},{"line_number":14,"context_line":"import os"},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"import mock"},{"line_number":17,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"5faad753_dae2b501","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":9},"in_reply_to":"5faad753_5a3b259b","updated":"2019-09-11 09:31:40.000000000","message":"I meant:\n\nimport os\n\\n\nimport mock\nimport .....\n\nDoesn\u0027t it work?","commit_id":"bbc372917b0e177a0bc840be8eb3878592055d71"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"5bd6e37cb535faec0bcfbf9499b02e22582d00b0","unresolved":false,"context_lines":[{"line_number":11,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":12,"context_line":"#    under the License."},{"line_number":13,"context_line":"#"},{"line_number":14,"context_line":"import os"},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"import mock"},{"line_number":17,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"5faad753_5a3b259b","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":9},"in_reply_to":"5faad753_941df6be","updated":"2019-09-11 09:24:38.000000000","message":"Yep, I tried :)\n\n\nWhen:\nimport mock\nimport os\n\n./networking_ovn/tests/unit/octavia/test_ovn_driver.py:15:1: I100 Import statements are in the wrong order. import os should be before import mock\nimport os\n\n\n\nWhen:\nimport os\nimport mock\n\n./networking_ovn/tests/unit/octavia/test_ovn_driver.py:15:1: H306  imports not in alphabetical order (os, mock)\nimport mock\n^\n./networking_ovn/tests/unit/octavia/test_ovn_driver.py:15:1: I201 Missing newline before sections or imports.\nimport mock\n\n\nSo the only one combination that actually works is the one I send, hmmm","commit_id":"bbc372917b0e177a0bc840be8eb3878592055d71"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"da58a50408ca7105a9db6661e83989006f872219","unresolved":false,"context_lines":[{"line_number":11,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":12,"context_line":"#    under the License."},{"line_number":13,"context_line":"#"},{"line_number":14,"context_line":"import os"},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"import mock"},{"line_number":17,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"5faad753_1a184d7f","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":9},"in_reply_to":"5faad753_da687522","updated":"2019-09-11 10:00:59.000000000","message":"Nice :)! Was a nit though :) but thanks","commit_id":"bbc372917b0e177a0bc840be8eb3878592055d71"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"475a154fb0b34dce7870cf2af96cb07ceaab8ce7","unresolved":false,"context_lines":[{"line_number":11,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":12,"context_line":"#    under the License."},{"line_number":13,"context_line":"#"},{"line_number":14,"context_line":"import os"},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"import mock"},{"line_number":17,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"5faad753_da687522","line":14,"range":{"start_line":14,"start_character":0,"end_line":14,"end_character":9},"in_reply_to":"5faad753_dae2b501","updated":"2019-09-11 09:47:26.000000000","message":"Works","commit_id":"bbc372917b0e177a0bc840be8eb3878592055d71"}]}