)]}'
{"neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py":[{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"ea1bf5fc82a10fa0a906a5ba408941f7083cd8b3","unresolved":true,"context_lines":[{"line_number":333,"context_line":"            # Call the synchronization task if its maintenance worker"},{"line_number":334,"context_line":"            # This sync neutron DB to OVN-NB DB only in inconsistent states"},{"line_number":335,"context_line":"            self._nb_ovn.idl.set_lock(\u0027ovn_db_inconsistencies_periodics\u0027)"},{"line_number":336,"context_line":"            self.nb_synchronizer \u003d ovn_db_sync.OvnNbSynchronizer("},{"line_number":337,"context_line":"                self._plugin,"},{"line_number":338,"context_line":"                self._nb_ovn,"},{"line_number":339,"context_line":"                self._sb_ovn,"},{"line_number":340,"context_line":"                ovn_conf.get_ovn_neutron_sync_mode(),"},{"line_number":341,"context_line":"                self"},{"line_number":342,"context_line":"            )"},{"line_number":343,"context_line":"            self.nb_synchronizer.sync()"},{"line_number":344,"context_line":""},{"line_number":345,"context_line":"            # This sync neutron DB to OVN-SB DB only in inconsistent states"},{"line_number":346,"context_line":"            self.sb_synchronizer \u003d ovn_db_sync.OvnSbSynchronizer("},{"line_number":347,"context_line":"                self._plugin,"},{"line_number":348,"context_line":"                self._sb_ovn,"},{"line_number":349,"context_line":"                self"},{"line_number":350,"context_line":"            )"},{"line_number":351,"context_line":"            self.sb_synchronizer.sync()"},{"line_number":352,"context_line":""},{"line_number":353,"context_line":"            self._maintenance_thread \u003d maintenance.MaintenanceThread()"},{"line_number":354,"context_line":"            self._maintenance_thread.add_periodics("}],"source_content_type":"text/x-python","patch_set":4,"id":"200b3bd6_eb93d9da","line":351,"range":{"start_line":336,"start_character":0,"end_line":351,"end_character":39},"updated":"2021-07-15 09:44:19.000000000","message":"I was wondering if it would be easier to check the lock at this part instead on the do_sync(). That way we wouldn\u0027t potentially need a lock for the SB DB (as per my comment) neither it would affect manually running the ovn-db-sync script.\n\nFor example:\n\nself._nb_ovn.idl.set_lock(\u0027ovn_db_inconsistencies_periodics\u0027)\nif self._nb_ovn.idl.has_lock:\n    self.nb_synchronizer \u003d ...\n    ...\n\nself._maintenance_thread \u003d maintenance.MaintenanceThread()\n...","commit_id":"b2a2e3202541c2a6357c177d6037ab313c3c3486"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"6d6dc9120535740aadc8d91e3f12617e7c53a5cf","unresolved":true,"context_lines":[{"line_number":333,"context_line":"            # Call the synchronization task if its maintenance worker"},{"line_number":334,"context_line":"            # This sync neutron DB to OVN-NB DB only in inconsistent states"},{"line_number":335,"context_line":"            self._nb_ovn.idl.set_lock(\u0027ovn_db_inconsistencies_periodics\u0027)"},{"line_number":336,"context_line":"            self.nb_synchronizer \u003d ovn_db_sync.OvnNbSynchronizer("},{"line_number":337,"context_line":"                self._plugin,"},{"line_number":338,"context_line":"                self._nb_ovn,"},{"line_number":339,"context_line":"                self._sb_ovn,"},{"line_number":340,"context_line":"                ovn_conf.get_ovn_neutron_sync_mode(),"},{"line_number":341,"context_line":"                self"},{"line_number":342,"context_line":"            )"},{"line_number":343,"context_line":"            self.nb_synchronizer.sync()"},{"line_number":344,"context_line":""},{"line_number":345,"context_line":"            # This sync neutron DB to OVN-SB DB only in inconsistent states"},{"line_number":346,"context_line":"            self.sb_synchronizer \u003d ovn_db_sync.OvnSbSynchronizer("},{"line_number":347,"context_line":"                self._plugin,"},{"line_number":348,"context_line":"                self._sb_ovn,"},{"line_number":349,"context_line":"                self"},{"line_number":350,"context_line":"            )"},{"line_number":351,"context_line":"            self.sb_synchronizer.sync()"},{"line_number":352,"context_line":""},{"line_number":353,"context_line":"            self._maintenance_thread \u003d maintenance.MaintenanceThread()"},{"line_number":354,"context_line":"            self._maintenance_thread.add_periodics("}],"source_content_type":"text/x-python","patch_set":4,"id":"80ba0d7c_63a81ae8","line":351,"range":{"start_line":336,"start_character":0,"end_line":351,"end_character":39},"in_reply_to":"200b3bd6_eb93d9da","updated":"2021-07-15 10:09:29.000000000","message":"Could be a possibility (only for the NB sync, not the SB sync).\n\nBut I prefer the \"nb_synchronizer.sync()\" method to check the lock inside, as is now (just a personal preference, the \"OvnNbSynchronizer\" should be aware of this lock).","commit_id":"b2a2e3202541c2a6357c177d6037ab313c3c3486"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"43cbc484648f506c5f492a47bc84d1e0d89dddda","unresolved":true,"context_lines":[{"line_number":333,"context_line":"            # Call the synchronization task if its maintenance worker"},{"line_number":334,"context_line":"            # This sync neutron DB to OVN-NB DB only in inconsistent states"},{"line_number":335,"context_line":"            self._nb_ovn.idl.set_lock(\u0027ovn_db_inconsistencies_periodics\u0027)"},{"line_number":336,"context_line":"            self.nb_synchronizer \u003d ovn_db_sync.OvnNbSynchronizer("},{"line_number":337,"context_line":"                self._plugin,"},{"line_number":338,"context_line":"                self._nb_ovn,"},{"line_number":339,"context_line":"                self._sb_ovn,"},{"line_number":340,"context_line":"                ovn_conf.get_ovn_neutron_sync_mode(),"},{"line_number":341,"context_line":"                self"},{"line_number":342,"context_line":"            )"},{"line_number":343,"context_line":"            self.nb_synchronizer.sync()"},{"line_number":344,"context_line":""},{"line_number":345,"context_line":"            # This sync neutron DB to OVN-SB DB only in inconsistent states"},{"line_number":346,"context_line":"            self.sb_synchronizer \u003d ovn_db_sync.OvnSbSynchronizer("},{"line_number":347,"context_line":"                self._plugin,"},{"line_number":348,"context_line":"                self._sb_ovn,"},{"line_number":349,"context_line":"                self"},{"line_number":350,"context_line":"            )"},{"line_number":351,"context_line":"            self.sb_synchronizer.sync()"},{"line_number":352,"context_line":""},{"line_number":353,"context_line":"            self._maintenance_thread \u003d maintenance.MaintenanceThread()"},{"line_number":354,"context_line":"            self._maintenance_thread.add_periodics("}],"source_content_type":"text/x-python","patch_set":4,"id":"2945ece5_93757374","line":351,"range":{"start_line":336,"start_character":0,"end_line":351,"end_character":39},"in_reply_to":"80ba0d7c_63a81ae8","updated":"2021-07-15 10:32:14.000000000","message":"Yeah I am indifferent, I thought here it would be simpler as we won\u0027t need to account for that has_lock/is_lock_contended in the script itself. Just when running within the service because that\u0027s when you could have multiple instances calling this method.","commit_id":"b2a2e3202541c2a6357c177d6037ab313c3c3486"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"0af6245bb0a1ce3be990387683643b49cc39b346","unresolved":true,"context_lines":[{"line_number":333,"context_line":"            # Call the synchronization task if its maintenance worker"},{"line_number":334,"context_line":"            # This sync neutron DB to OVN-NB DB only in inconsistent states"},{"line_number":335,"context_line":"            self._nb_ovn.idl.set_lock(\u0027ovn_db_inconsistencies_periodics\u0027)"},{"line_number":336,"context_line":"            if not self._nb_ovn.idl.is_lock_contended and self._nb_ovn.idl.has_lock:"},{"line_number":337,"context_line":"                self.nb_synchronizer \u003d ovn_db_sync.OvnNbSynchronizer("},{"line_number":338,"context_line":"                    self._plugin,"},{"line_number":339,"context_line":"                    self._nb_ovn,"}],"source_content_type":"text/x-python","patch_set":6,"id":"b9aedf3e_375d7cd5","line":336,"range":{"start_line":336,"start_character":58,"end_line":336,"end_character":83},"updated":"2021-07-15 12:56:59.000000000","message":"Unnecessary: the idl has a configured lock, you created it in the previous line.","commit_id":"fed179e8a22f30ef98042d273e094de96dc117b3"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"953c2cec07e36ddd572573c6ea890570f238e53a","unresolved":true,"context_lines":[{"line_number":97,"context_line":"        LOG.debug(\"Starting OVN-Northbound DB sync process\")"},{"line_number":98,"context_line":""},{"line_number":99,"context_line":"        ctx \u003d context.get_admin_context()"},{"line_number":100,"context_line":"        if not self.ovn_api.idl.has_lock:"},{"line_number":101,"context_line":"            return"},{"line_number":102,"context_line":"        self.sync_port_groups(ctx)"},{"line_number":103,"context_line":"        self.sync_networks_ports_and_dhcp_opts(ctx)"},{"line_number":104,"context_line":"        self.sync_port_dns_records(ctx)"}],"source_content_type":"text/x-python","patch_set":1,"id":"c1ef77fd_f2044797","line":101,"range":{"start_line":100,"start_character":8,"end_line":101,"end_character":18},"updated":"2021-07-14 08:28:52.000000000","message":"I agree that the issue reported is a problem in \"MaintenanceWorker\".\n\nI have two concerns here. As commented in the LP bug, \"MaintenanceWorker\" execute two different operations:\n1) the sync between the Neutron DB and the OVN DB, done by \"OvnNbSynchronizer\"\n2) the inconsistencies repair, executed by \"DBInconsistenciesPeriodics\"\n\nBoth classes, executed in parallel, use the same IDL instance with the same lock. My concern here is that because a \"DBInconsistenciesPeriodics\" task is being executed, no other server could execute the \"OvnNbSynchronizer\" tasks.\n\nThe \"DBInconsistenciesPeriodics\" are executed periodically, that means if we skip one execution, we\u0027ll try again. But that doesn\u0027t happen with \"OvnNbSynchronizer\" tasks (from L102 to L106). If no server has the lock to execute the sync methods, we\u0027ll never execute them.\n\nI would like Terry and Lucas to take a look at this patch and check if what I\u0027m saying is correct.","commit_id":"c479d79edfc297d87d97a8ea1fa0d3dafec36d32"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"ea1bf5fc82a10fa0a906a5ba408941f7083cd8b3","unresolved":true,"context_lines":[{"line_number":98,"context_line":""},{"line_number":99,"context_line":"        ctx \u003d context.get_admin_context()"},{"line_number":100,"context_line":"        if not self.ovn_api.idl.has_lock:"},{"line_number":101,"context_line":"            return"},{"line_number":102,"context_line":"        self.sync_port_groups(ctx)"},{"line_number":103,"context_line":"        self.sync_networks_ports_and_dhcp_opts(ctx)"},{"line_number":104,"context_line":"        self.sync_port_dns_records(ctx)"}],"source_content_type":"text/x-python","patch_set":4,"id":"4edbd1cb_e81e5bb0","line":101,"updated":"2021-07-15 09:44:19.000000000","message":"We also call this method when running the ovn-db-sync script manually, did we check it to make sure that this lock check does not affect it ? \n\nhttps://github.com/openstack/neutron/blob/603951809a45797697135e03eb4870c094371480/neutron/cmd/ovn/neutron_ovn_db_sync_util.py#L235-L242","commit_id":"b2a2e3202541c2a6357c177d6037ab313c3c3486"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"43cbc484648f506c5f492a47bc84d1e0d89dddda","unresolved":true,"context_lines":[{"line_number":98,"context_line":""},{"line_number":99,"context_line":"        ctx \u003d context.get_admin_context()"},{"line_number":100,"context_line":"        if not self.ovn_api.idl.has_lock:"},{"line_number":101,"context_line":"            return"},{"line_number":102,"context_line":"        self.sync_port_groups(ctx)"},{"line_number":103,"context_line":"        self.sync_networks_ports_and_dhcp_opts(ctx)"},{"line_number":104,"context_line":"        self.sync_port_dns_records(ctx)"}],"source_content_type":"text/x-python","patch_set":4,"id":"a2a40472_35d382dc","line":101,"in_reply_to":"004249df_ce4c88b7","updated":"2021-07-15 10:32:14.000000000","message":"Right, so using has_lock here is a problem because when running the script manually won\u0027t work because the IDL from ovn-db-sync script hasn\u0027t created a lock. Indeed we should check for is_lock_contended here or a combination of the two to account for the manual usage of the script as well","commit_id":"b2a2e3202541c2a6357c177d6037ab313c3c3486"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"6d6dc9120535740aadc8d91e3f12617e7c53a5cf","unresolved":true,"context_lines":[{"line_number":98,"context_line":""},{"line_number":99,"context_line":"        ctx \u003d context.get_admin_context()"},{"line_number":100,"context_line":"        if not self.ovn_api.idl.has_lock:"},{"line_number":101,"context_line":"            return"},{"line_number":102,"context_line":"        self.sync_port_groups(ctx)"},{"line_number":103,"context_line":"        self.sync_networks_ports_and_dhcp_opts(ctx)"},{"line_number":104,"context_line":"        self.sync_port_dns_records(ctx)"}],"source_content_type":"text/x-python","patch_set":4,"id":"004249df_ce4c88b7","line":101,"in_reply_to":"4edbd1cb_e81e5bb0","updated":"2021-07-15 10:09:29.000000000","message":"If there is not lock defined or it was removed, \"Idl.hash_lock\" is False.\n\nThe problem here is that you should check idl.is_lock_contended. There is a difference between idl.has_lock and idl.is_lock_contended [1]. This is what \"DBInconsistenciesPeriodics\" checks [2]\n\n\n[1]https://github.com/openvswitch/ovs/blob/d2e97030eda5dfafc875ec056788c49e2570d2ef/python/ovs/db/idl.py#L169-L170\n[2]https://github.com/openstack/neutron/blob/3cae410b3094e74549a1941e0b7d833229ae51d5/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py#L195-L197","commit_id":"b2a2e3202541c2a6357c177d6037ab313c3c3486"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"ea1bf5fc82a10fa0a906a5ba408941f7083cd8b3","unresolved":true,"context_lines":[{"line_number":1235,"context_line":"        is."},{"line_number":1236,"context_line":"        \"\"\""},{"line_number":1237,"context_line":"        LOG.debug(\"Starting OVN-Southbound DB sync process\")"},{"line_number":1238,"context_line":""},{"line_number":1239,"context_line":"        ctx \u003d context.get_admin_context()"},{"line_number":1240,"context_line":"        self.sync_hostname_and_physical_networks(ctx)"},{"line_number":1241,"context_line":"        if utils.is_ovn_l3(self.l3_plugin):"}],"source_content_type":"text/x-python","patch_set":4,"id":"db2b7e13_9ba064e1","line":1238,"updated":"2021-07-15 09:44:19.000000000","message":"Q: Do we also need a lock for SB DB here ?","commit_id":"b2a2e3202541c2a6357c177d6037ab313c3c3486"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"6d6dc9120535740aadc8d91e3f12617e7c53a5cf","unresolved":true,"context_lines":[{"line_number":1235,"context_line":"        is."},{"line_number":1236,"context_line":"        \"\"\""},{"line_number":1237,"context_line":"        LOG.debug(\"Starting OVN-Southbound DB sync process\")"},{"line_number":1238,"context_line":""},{"line_number":1239,"context_line":"        ctx \u003d context.get_admin_context()"},{"line_number":1240,"context_line":"        self.sync_hostname_and_physical_networks(ctx)"},{"line_number":1241,"context_line":"        if utils.is_ovn_l3(self.l3_plugin):"}],"source_content_type":"text/x-python","patch_set":4,"id":"fe81d7ce_6da4a2ef","line":1238,"in_reply_to":"db2b7e13_9ba064e1","updated":"2021-07-15 10:09:29.000000000","message":"We don\u0027t create a lock for the SB idl in the \"DBInconsistenciesPeriodics\" class.\n\nHowever, as in NB DB, there is no need to execute those commands from all servers. I think that is out of scope from this patch but something that we should implement.","commit_id":"b2a2e3202541c2a6357c177d6037ab313c3c3486"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"b5f59f80eaab019eaca96aa54d50a543089027d1","unresolved":true,"context_lines":[{"line_number":1235,"context_line":"        is."},{"line_number":1236,"context_line":"        \"\"\""},{"line_number":1237,"context_line":"        LOG.debug(\"Starting OVN-Southbound DB sync process\")"},{"line_number":1238,"context_line":""},{"line_number":1239,"context_line":"        ctx \u003d context.get_admin_context()"},{"line_number":1240,"context_line":"        self.sync_hostname_and_physical_networks(ctx)"},{"line_number":1241,"context_line":"        if utils.is_ovn_l3(self.l3_plugin):"}],"source_content_type":"text/x-python","patch_set":4,"id":"69b36631_14a8196a","line":1238,"in_reply_to":"fe81d7ce_6da4a2ef","updated":"2021-07-15 10:33:25.000000000","message":"Yeah DBInconsistenciesPeriodics doesn\u0027t touch the south database, but this does. That\u0027s why I think we should guard it too.","commit_id":"b2a2e3202541c2a6357c177d6037ab313c3c3486"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"3c633a59cd8a04c076d88fe38e0076e77879f7d8","unresolved":true,"context_lines":[{"line_number":52,"context_line":"    def sync(self, delay_seconds\u003d10):"},{"line_number":53,"context_line":"        self._gt \u003d greenthread.spawn_after_local(delay_seconds, self.do_sync)"},{"line_number":54,"context_line":"        if not self.ovn_api.idl.is_lock_contended:"},{"line_number":55,"context_line":"            return"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    @abc.abstractmethod"},{"line_number":58,"context_line":"    def do_sync(self):"}],"source_content_type":"text/x-python","patch_set":5,"id":"803b987c_d3c00a73","line":55,"updated":"2021-07-15 12:21:19.000000000","message":"I don\u0027t think this has any effect ? This is the end of the method scope after the greenthread has already been spawned.\n\nAlso I am not sure about this is_lock_contended, from what I understood this will return False when we do not ask for any locks (and we do not create a lock when running the script manually).\n\nThat\u0027s why I thought that doing such checks on post_fork_initialize() is better cause we only care about locks at that point not for the manual case.","commit_id":"7d73ba5e4f2972571386b0793e7c2f4009cba12f"}]}