)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"bd4bb0a59adc8f30e5d6b82143838a1b7c141c8f","unresolved":true,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"As Neutron is storing a liveness_check in the NB_Global table,"},{"line_number":10,"context_line":"every once in a while, it will possibly run sync more often"},{"line_number":11,"context_line":"than wanted."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Closes-Bug: #2096661"},{"line_number":14,"context_line":"Change-Id: I62fc85f757b8ba35f8fa6bff873b2f4ca18206d8"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"b5eca7a7_2284515a","line":11,"updated":"2025-01-28 14:10:55.000000000","message":"It took me a while to realize why it\u0027s running - is it because your Neutron is missing https://code.engineering.redhat.com/gerrit/c/neutron/+/454095 ?","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"524a5a0baa3c5c1bd7f7e3f7b0b8c8dd9a2d9030","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"As Neutron is storing a liveness_check in the NB_Global table,"},{"line_number":10,"context_line":"every once in a while, it will possibly run sync more often"},{"line_number":11,"context_line":"than wanted."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Closes-Bug: #2096661"},{"line_number":14,"context_line":"Change-Id: I62fc85f757b8ba35f8fa6bff873b2f4ca18206d8"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"3076f244_01912d3b","line":11,"in_reply_to":"3028ebe3_c9ca2a70","updated":"2025-01-29 13:21:23.000000000","message":"Apologies, my bad, used internal link :-/ This is the right one https://review.opendev.org/c/openstack/neutron/+/935207/1\n\nYes, it\u0027s in 2023.2 only. Still, this patch is valid, the code should be defensive enough to handle your situation.","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"},{"author":{"_account_id":25468,"name":"Michel Nederlof","email":"michel@nederlof.info","username":"pellucid"},"change_message_id":"63222cebfb3ad675bc56c64b3ae11e0988bb996c","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"As Neutron is storing a liveness_check in the NB_Global table,"},{"line_number":10,"context_line":"every once in a while, it will possibly run sync more often"},{"line_number":11,"context_line":"than wanted."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Closes-Bug: #2096661"},{"line_number":14,"context_line":"Change-Id: I62fc85f757b8ba35f8fa6bff873b2f4ca18206d8"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"3028ebe3_c9ca2a70","line":11,"in_reply_to":"b5eca7a7_2284515a","updated":"2025-01-29 08:53:47.000000000","message":"This url does not work for me unfortunately, but we are still running neutron 2023.1, with ovn-bgp-agent 2024.2, so probably some fields are missing, yes.","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"bd4bb0a59adc8f30e5d6b82143838a1b7c141c8f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"3c4d9adc_bacb2625","updated":"2025-01-28 14:10:55.000000000","message":"Some comments - I think when I wrote the original code I didn\u0027t count with the Neutron patch missing and therefore the distributed flag would be missing on each update. It would be great to add this scenario to the functional tests.","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"}],"ovn_bgp_agent/drivers/openstack/watchers/nb_bgp_watcher.py":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"bd4bb0a59adc8f30e5d6b82143838a1b7c141c8f","unresolved":true,"context_lines":[{"line_number":1053,"context_line":"    def match_fn(self, event, row, old):"},{"line_number":1054,"context_line":"        try:"},{"line_number":1055,"context_line":"            if (old.external_ids.get(constants.OVN_FIP_DISTRIBUTED) \u003d\u003d"},{"line_number":1056,"context_line":"                    row.external_ids[constants.OVN_FIP_DISTRIBUTED]):"},{"line_number":1057,"context_line":"                return False"},{"line_number":1058,"context_line":"        except KeyError:"},{"line_number":1059,"context_line":"            # Distributed flag was deleted, behave like distributed agent"}],"source_content_type":"text/x-python","patch_set":3,"id":"5d5acb45_90571cb7","side":"PARENT","line":1056,"range":{"start_line":1056,"start_character":35,"end_line":1056,"end_character":67},"updated":"2025-01-28 14:10:55.000000000","message":"Shouldn\u0027t it be enough to just change this call to `get()` and remove the KeyError?","commit_id":"f90262e7d38c3af7ce220d94f8774e17295b496f"},{"author":{"_account_id":25468,"name":"Michel Nederlof","email":"michel@nederlof.info","username":"pellucid"},"change_message_id":"63222cebfb3ad675bc56c64b3ae11e0988bb996c","unresolved":false,"context_lines":[{"line_number":1053,"context_line":"    def match_fn(self, event, row, old):"},{"line_number":1054,"context_line":"        try:"},{"line_number":1055,"context_line":"            if (old.external_ids.get(constants.OVN_FIP_DISTRIBUTED) \u003d\u003d"},{"line_number":1056,"context_line":"                    row.external_ids[constants.OVN_FIP_DISTRIBUTED]):"},{"line_number":1057,"context_line":"                return False"},{"line_number":1058,"context_line":"        except KeyError:"},{"line_number":1059,"context_line":"            # Distributed flag was deleted, behave like distributed agent"}],"source_content_type":"text/x-python","patch_set":3,"id":"83db8908_95a9d9c4","side":"PARENT","line":1056,"range":{"start_line":1056,"start_character":35,"end_line":1056,"end_character":67},"in_reply_to":"5d5acb45_90571cb7","updated":"2025-01-29 08:53:47.000000000","message":"maybe it is, yet i think it reads easier to have the try/except removed, as there cannot/shouldnt be a attribute/key error anymore as we check for the attribute to be there before checking the values.\n\nAnd in that case it is for me easier to interpret the code like i propose here.\n(also, i do not like relying on a return True as last resort for this event, as it will run the sync method)","commit_id":"f90262e7d38c3af7ce220d94f8774e17295b496f"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"524a5a0baa3c5c1bd7f7e3f7b0b8c8dd9a2d9030","unresolved":true,"context_lines":[{"line_number":1053,"context_line":"    def match_fn(self, event, row, old):"},{"line_number":1054,"context_line":"        try:"},{"line_number":1055,"context_line":"            if (old.external_ids.get(constants.OVN_FIP_DISTRIBUTED) \u003d\u003d"},{"line_number":1056,"context_line":"                    row.external_ids[constants.OVN_FIP_DISTRIBUTED]):"},{"line_number":1057,"context_line":"                return False"},{"line_number":1058,"context_line":"        except KeyError:"},{"line_number":1059,"context_line":"            # Distributed flag was deleted, behave like distributed agent"}],"source_content_type":"text/x-python","patch_set":3,"id":"93bd42c6_ec03b68b","side":"PARENT","line":1056,"range":{"start_line":1056,"start_character":35,"end_line":1056,"end_character":67},"in_reply_to":"83db8908_95a9d9c4","updated":"2025-01-29 13:21:23.000000000","message":"That leads us to discussion when one prefers LBYL. EAFP style is more common for Python - https://docs.python.org/3/glossary.html#term-EAFP","commit_id":"f90262e7d38c3af7ce220d94f8774e17295b496f"},{"author":{"_account_id":25468,"name":"Michel Nederlof","email":"michel@nederlof.info","username":"pellucid"},"change_message_id":"e096f02bb374bf1b0e3d9604d08c28391345f993","unresolved":false,"context_lines":[{"line_number":1053,"context_line":"    def match_fn(self, event, row, old):"},{"line_number":1054,"context_line":"        try:"},{"line_number":1055,"context_line":"            if (old.external_ids.get(constants.OVN_FIP_DISTRIBUTED) \u003d\u003d"},{"line_number":1056,"context_line":"                    row.external_ids[constants.OVN_FIP_DISTRIBUTED]):"},{"line_number":1057,"context_line":"                return False"},{"line_number":1058,"context_line":"        except KeyError:"},{"line_number":1059,"context_line":"            # Distributed flag was deleted, behave like distributed agent"}],"source_content_type":"text/x-python","patch_set":3,"id":"dfa2dad4_ece63357","side":"PARENT","line":1056,"range":{"start_line":1056,"start_character":35,"end_line":1056,"end_character":67},"in_reply_to":"93bd42c6_ec03b68b","updated":"2025-01-30 14:46:49.000000000","message":"Not trying to do that discussion, i just believe this code was better to read and shorter in lines. i will update it back with some adjustments to see if it would actually change the distributed flag and only then return True.","commit_id":"f90262e7d38c3af7ce220d94f8774e17295b496f"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"bd4bb0a59adc8f30e5d6b82143838a1b7c141c8f","unresolved":true,"context_lines":[{"line_number":1051,"context_line":"        self.event_name \u003d self.__class__.__name__"},{"line_number":1052,"context_line":""},{"line_number":1053,"context_line":"    def match_fn(self, event, row, old):"},{"line_number":1054,"context_line":"        if getattr(old, \u0027external_ids\u0027, None) is None:"},{"line_number":1055,"context_line":"            # no update on external_ids"},{"line_number":1056,"context_line":"            return False"},{"line_number":1057,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"85a903de_cf159f89","line":1054,"range":{"start_line":1054,"start_character":11,"end_line":1054,"end_character":53},"updated":"2025-01-28 14:10:55.000000000","message":"I\u0027m not sure I understand this condition - shouldn\u0027t it be just `if not hasattr(old, \u0027external_ids\u0027)` ?","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"},{"author":{"_account_id":25468,"name":"Michel Nederlof","email":"michel@nederlof.info","username":"pellucid"},"change_message_id":"63222cebfb3ad675bc56c64b3ae11e0988bb996c","unresolved":false,"context_lines":[{"line_number":1051,"context_line":"        self.event_name \u003d self.__class__.__name__"},{"line_number":1052,"context_line":""},{"line_number":1053,"context_line":"    def match_fn(self, event, row, old):"},{"line_number":1054,"context_line":"        if getattr(old, \u0027external_ids\u0027, None) is None:"},{"line_number":1055,"context_line":"            # no update on external_ids"},{"line_number":1056,"context_line":"            return False"},{"line_number":1057,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"4077fce0_6373e9c4","line":1054,"range":{"start_line":1054,"start_character":11,"end_line":1054,"end_character":53},"in_reply_to":"85a903de_cf159f89","updated":"2025-01-29 08:53:47.000000000","message":"yes, that could work too 😊","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"}],"ovn_bgp_agent/tests/unit/drivers/openstack/watchers/test_nb_bgp_watcher.py":[{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"bd4bb0a59adc8f30e5d6b82143838a1b7c141c8f","unresolved":true,"context_lines":[{"line_number":1719,"context_line":"        self.assertTrue(self._call_match(old))"},{"line_number":1720,"context_line":""},{"line_number":1721,"context_line":""},{"line_number":1722,"context_line":"class TestDistributedFlagChangedEvent(test_base.TestCase):"},{"line_number":1723,"context_line":"    def setUp(self):"},{"line_number":1724,"context_line":"        super(TestDistributedFlagChangedEvent, self).setUp()"},{"line_number":1725,"context_line":"        self.chassis \u003d \"fake-chassis\""}],"source_content_type":"text/x-python","patch_set":3,"id":"e99662ae_45265503","line":1722,"range":{"start_line":1722,"start_character":0,"end_line":1722,"end_character":58},"updated":"2025-01-28 14:10:55.000000000","message":"The event is covered by functional tests - https://github.com/openstack/ovn-bgp-agent/blob/f90262e7d38c3af7ce220d94f8774e17295b496f/ovn_bgp_agent/tests/functional/drivers/openstack/watchers/test_nb_bgp_watcher.py#L36 \n\nwhich I think is the right way to go to make sure we really re-act on events. Is there a reason to introduce these unittests?","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"296d6da1258ea02c0c6dfb27306ea993cbca531b","unresolved":false,"context_lines":[{"line_number":1719,"context_line":"        self.assertTrue(self._call_match(old))"},{"line_number":1720,"context_line":""},{"line_number":1721,"context_line":""},{"line_number":1722,"context_line":"class TestDistributedFlagChangedEvent(test_base.TestCase):"},{"line_number":1723,"context_line":"    def setUp(self):"},{"line_number":1724,"context_line":"        super(TestDistributedFlagChangedEvent, self).setUp()"},{"line_number":1725,"context_line":"        self.chassis \u003d \"fake-chassis\""}],"source_content_type":"text/x-python","patch_set":3,"id":"d8a17073_ef6bdc34","line":1722,"range":{"start_line":1722,"start_character":0,"end_line":1722,"end_character":58},"in_reply_to":"5914cf8a_bb4ba3b5","updated":"2025-01-30 20:07:37.000000000","message":"I\u0027ll be happy to help with that :) I think just sending updates of external_ids while distributed key not being either in the database or in the update and then checking sync was not called is a good test case.","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"},{"author":{"_account_id":25468,"name":"Michel Nederlof","email":"michel@nederlof.info","username":"pellucid"},"change_message_id":"e096f02bb374bf1b0e3d9604d08c28391345f993","unresolved":false,"context_lines":[{"line_number":1719,"context_line":"        self.assertTrue(self._call_match(old))"},{"line_number":1720,"context_line":""},{"line_number":1721,"context_line":""},{"line_number":1722,"context_line":"class TestDistributedFlagChangedEvent(test_base.TestCase):"},{"line_number":1723,"context_line":"    def setUp(self):"},{"line_number":1724,"context_line":"        super(TestDistributedFlagChangedEvent, self).setUp()"},{"line_number":1725,"context_line":"        self.chassis \u003d \"fake-chassis\""}],"source_content_type":"text/x-python","patch_set":3,"id":"5914cf8a_bb4ba3b5","line":1722,"range":{"start_line":1722,"start_character":0,"end_line":1722,"end_character":58},"in_reply_to":"6aec944d_8f9c1a58","updated":"2025-01-30 14:46:49.000000000","message":"I\u0027ve tried to add a check on the functional test as well, though i was a bit struggling there to see how i could match this scenario.","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"524a5a0baa3c5c1bd7f7e3f7b0b8c8dd9a2d9030","unresolved":true,"context_lines":[{"line_number":1719,"context_line":"        self.assertTrue(self._call_match(old))"},{"line_number":1720,"context_line":""},{"line_number":1721,"context_line":""},{"line_number":1722,"context_line":"class TestDistributedFlagChangedEvent(test_base.TestCase):"},{"line_number":1723,"context_line":"    def setUp(self):"},{"line_number":1724,"context_line":"        super(TestDistributedFlagChangedEvent, self).setUp()"},{"line_number":1725,"context_line":"        self.chassis \u003d \"fake-chassis\""}],"source_content_type":"text/x-python","patch_set":3,"id":"6aec944d_8f9c1a58","line":1722,"range":{"start_line":1722,"start_character":0,"end_line":1722,"end_character":58},"in_reply_to":"b859e495_38b9da04","updated":"2025-01-29 13:21:23.000000000","message":"Keep in mind unittests require maintenance too. I think it makes sense to have such unittests in situations in which we do not have functional tests yet - because we started having that until quite recently. This particular event has the coverage in place - seems like it\u0027s missing the test case where distributed is never set but updates keep coming.\n\nThe downside of these unittests is that they are prone to typos - we create the row column names and values on our own as opposed from the functional tests the column names come from the OVN DBs schema.","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"},{"author":{"_account_id":25468,"name":"Michel Nederlof","email":"michel@nederlof.info","username":"pellucid"},"change_message_id":"714a8a55239b10d5d7dba3b15200b793168edf23","unresolved":false,"context_lines":[{"line_number":1719,"context_line":"        self.assertTrue(self._call_match(old))"},{"line_number":1720,"context_line":""},{"line_number":1721,"context_line":""},{"line_number":1722,"context_line":"class TestDistributedFlagChangedEvent(test_base.TestCase):"},{"line_number":1723,"context_line":"    def setUp(self):"},{"line_number":1724,"context_line":"        super(TestDistributedFlagChangedEvent, self).setUp()"},{"line_number":1725,"context_line":"        self.chassis \u003d \"fake-chassis\""}],"source_content_type":"text/x-python","patch_set":3,"id":"ccc91799_a8c8e61a","line":1722,"range":{"start_line":1722,"start_character":0,"end_line":1722,"end_character":58},"in_reply_to":"d8a17073_ef6bdc34","updated":"2025-02-07 14:45:52.000000000","message":"Had to change the test, as the assertTrue on the inherited test event matching did not always work properly, as the real DistributedFlagChangedEvent could have already changed the agent distributed flag before the match_fn of the WaitEvent was called.\n\nTherefore i introduced a state check on the distributed flag itself as well, to make sure the test case is consistent.\n\nHopefully it passes zuul testing now as well, as the test is passing at my end 😊","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"},{"author":{"_account_id":25468,"name":"Michel Nederlof","email":"michel@nederlof.info","username":"pellucid"},"change_message_id":"63222cebfb3ad675bc56c64b3ae11e0988bb996c","unresolved":false,"context_lines":[{"line_number":1719,"context_line":"        self.assertTrue(self._call_match(old))"},{"line_number":1720,"context_line":""},{"line_number":1721,"context_line":""},{"line_number":1722,"context_line":"class TestDistributedFlagChangedEvent(test_base.TestCase):"},{"line_number":1723,"context_line":"    def setUp(self):"},{"line_number":1724,"context_line":"        super(TestDistributedFlagChangedEvent, self).setUp()"},{"line_number":1725,"context_line":"        self.chassis \u003d \"fake-chassis\""}],"source_content_type":"text/x-python","patch_set":3,"id":"b859e495_38b9da04","line":1722,"range":{"start_line":1722,"start_character":0,"end_line":1722,"end_character":58},"in_reply_to":"e99662ae_45265503","updated":"2025-01-29 08:53:47.000000000","message":"It was for me just easier to write unit tests for the match_fn method. \n\nAnd to me functional tests are a addition to the unit tests, to see if all event fire as requested, while with unit tests, you check if the method reacts as expected for the usecase described\n\nWhich is exactly what i want to test for this specific change, as it only implies the match_fn.","commit_id":"caa646494e7cfcab532676d4972efe8b8befee30"}]}