)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"d19fb34ce02a6f2f126a76da5c1a76539905469b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"c85a80aa_bd9f7122","updated":"2025-06-11 21:59:08.000000000","message":"Mostly looks good to me and the ovsdb stuff does what it says it\u0027s doing. Nice work! Just some questions on some of the testing.","commit_id":"025a13bd2141cfd84117ba470c9cd5792c241322"}],"ovn_octavia_provider/event.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"d19fb34ce02a6f2f126a76da5c1a76539905469b","unresolved":false,"context_lines":[{"line_number":86,"context_line":"        self.driver \u003d driver"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    def match_fn(self, event, row, old):"},{"line_number":89,"context_line":"        if event \u003d\u003d self.ROW_CREATE:"},{"line_number":90,"context_line":"            chassis_name \u003d getattr(row, \u0027chassis_name\u0027, \u0027\u0027)"},{"line_number":91,"context_line":"            return not chassis_name"},{"line_number":92,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":4,"id":"88e395f2_6e9b1712","line":89,"updated":"2025-06-11 21:59:08.000000000","message":"We have seen cases in neutron where if notifications are slow to a client and a CREATE and an UPDATE have both been done before the client receives the UPDATE notification, the notification will come as a CREATE *with the updated data from the update*.\n\nI don\u0027t know if this is a likely scenario for octavia (maybe not since there was no CREATE handling here before anyway), but if it is then it might be good to handle create in a way that detects the situation and calls the appropriate code.","commit_id":"025a13bd2141cfd84117ba470c9cd5792c241322"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"39e74b41fe9ee62768242fba6035d30bc6c0858c","unresolved":false,"context_lines":[{"line_number":86,"context_line":"        self.driver \u003d driver"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"    def match_fn(self, event, row, old):"},{"line_number":89,"context_line":"        if event \u003d\u003d self.ROW_CREATE:"},{"line_number":90,"context_line":"            chassis_name \u003d getattr(row, \u0027chassis_name\u0027, \u0027\u0027)"},{"line_number":91,"context_line":"            return not chassis_name"},{"line_number":92,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":4,"id":"d88bb055_9934cf19","line":89,"in_reply_to":"88e395f2_6e9b1712","updated":"2025-06-17 09:01:47.000000000","message":"The update we\u0027re interested in occurs when the status changes, and this status change happens after the configured number of checks have been sent at the specified interval. Therefore, I understand that this overlap of events shouldn\u0027t occur here. In any case, we can cover it with an additional check for both status and chassis being set during a creation event, and call the method that handles the update.","commit_id":"025a13bd2141cfd84117ba470c9cd5792c241322"}],"ovn_octavia_provider/helper.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"d19fb34ce02a6f2f126a76da5c1a76539905469b","unresolved":false,"context_lines":[{"line_number":3984,"context_line":"        # that unreachable backend member, we will set manually the status to"},{"line_number":3985,"context_line":"        # \u0027offline\u0027, at this way no traffic will be redirected to it and trust"},{"line_number":3986,"context_line":"        # on update when the VM is re/started."},{"line_number":3987,"context_line":"        cmd \u003d ["},{"line_number":3988,"context_line":"            \"OVN_Southbound\", {"},{"line_number":3989,"context_line":"                \"op\": \"update\","},{"line_number":3990,"context_line":"                \"table\": \"Service_Monitor\","}],"source_content_type":"text/x-python","patch_set":4,"id":"7e77cde7_0469156f","line":3987,"updated":"2025-06-11 21:59:08.000000000","message":"I tested this manually and it seems to safely do what it\u0027s supposed to do. I initially though we\u0027d need to use \"mutate\", but I was wrong!","commit_id":"025a13bd2141cfd84117ba470c9cd5792c241322"}],"ovn_octavia_provider/ovsdb/impl_idl_ovn.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"d19fb34ce02a6f2f126a76da5c1a76539905469b","unresolved":false,"context_lines":[{"line_number":270,"context_line":"        self.close()"},{"line_number":271,"context_line":""},{"line_number":272,"context_line":""},{"line_number":273,"context_line":"class OvsdbClientCommand:"},{"line_number":274,"context_line":"    _CONNECTION \u003d 0"},{"line_number":275,"context_line":"    _PRIVATE_KEY \u003d 1"},{"line_number":276,"context_line":"    _CERTIFICATE \u003d 2"}],"source_content_type":"text/x-python","patch_set":4,"id":"0135290d_34c18303","line":273,"updated":"2025-06-11 21:59:08.000000000","message":"Eventually I guess this should go in neutron-lib since it\u0027s used in neutron and ovn-octavia-provider now. But that can happen later. I\u0027ll also try to look into doing some kind of light-weight connection with python-ovs that doesn\u0027t subscribe to anything and lets us send custom ops via https://patchwork.ozlabs.org/project/openvswitch/patch/20240628191841.2035262-1-twilson@redhat.com/","commit_id":"025a13bd2141cfd84117ba470c9cd5792c241322"}],"ovn_octavia_provider/tests/unit/ovsdb/test_impl_idl_ovn.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"d19fb34ce02a6f2f126a76da5c1a76539905469b","unresolved":true,"context_lines":[{"line_number":106,"context_line":"                               \u0027set_lock\u0027) as set_lock:"},{"line_number":107,"context_line":"            self.idl \u003d impl_idl_ovn.OvnSbIdlForLb(event_lock_name\u003d\u0027foo\u0027)"},{"line_number":108,"context_line":"        set_lock.assert_called_once_with(\u0027foo\u0027)"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"class TestOvnNbTransaction(base.BaseTestCase):"},{"line_number":112,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"bee41714_c13a00f8","line":109,"updated":"2025-06-11 21:59:08.000000000","message":"Are the changes in this file related to this patch?","commit_id":"025a13bd2141cfd84117ba470c9cd5792c241322"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"39e74b41fe9ee62768242fba6035d30bc6c0858c","unresolved":true,"context_lines":[{"line_number":106,"context_line":"                               \u0027set_lock\u0027) as set_lock:"},{"line_number":107,"context_line":"            self.idl \u003d impl_idl_ovn.OvnSbIdlForLb(event_lock_name\u003d\u0027foo\u0027)"},{"line_number":108,"context_line":"        set_lock.assert_called_once_with(\u0027foo\u0027)"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"class TestOvnNbTransaction(base.BaseTestCase):"},{"line_number":112,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"bb595f36_e9e717e7","line":109,"in_reply_to":"bee41714_c13a00f8","updated":"2025-06-17 09:01:47.000000000","message":"It was an addition to increase the file\u0027s coverage to make sure that, with the lines added, we were still exceeding the coverage threshold. It\u0027s a bit high at 92%, and with even the slightest line added, it marks the gates in red...","commit_id":"025a13bd2141cfd84117ba470c9cd5792c241322"}],"ovn_octavia_provider/tests/unit/test_helper.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"d19fb34ce02a6f2f126a76da5c1a76539905469b","unresolved":true,"context_lines":[{"line_number":6820,"context_line":""},{"line_number":6821,"context_line":"        result \u003d self.helper.force_status_offline_service_monitor(fake_row)"},{"line_number":6822,"context_line":""},{"line_number":6823,"context_line":"        expected_cmd \u003d ["},{"line_number":6824,"context_line":"            \"OVN_Southbound\", {"},{"line_number":6825,"context_line":"                \"op\": \"update\","},{"line_number":6826,"context_line":"                \"table\": \"Service_Monitor\","}],"source_content_type":"text/x-python","patch_set":4,"id":"e3f6cdd1_ca5a460b","line":6823,"updated":"2025-06-11 21:59:08.000000000","message":"This test just looks like writing code twice. If anything in the function changed at all, the test would just have to mirror that change. Too much reliance on implementation details IMHO.\n\nIt\u0027s the kind of thing I\u0027d much rather see a functional test cover so the test could be \"if force_status_offline_success() is called, the Serivce_Monitor status is set to offline\" as opposed to \"I\u0027m sure I wrote it correctly the first time I wrote it.\" (I will admit a bias to thinking that nearly all ovsdb-related tests should be functional tests, though.)","commit_id":"025a13bd2141cfd84117ba470c9cd5792c241322"},{"author":{"_account_id":34451,"name":"Fernando Royo","email":"froyo@redhat.com","username":"froyo"},"change_message_id":"39e74b41fe9ee62768242fba6035d30bc6c0858c","unresolved":true,"context_lines":[{"line_number":6820,"context_line":""},{"line_number":6821,"context_line":"        result \u003d self.helper.force_status_offline_service_monitor(fake_row)"},{"line_number":6822,"context_line":""},{"line_number":6823,"context_line":"        expected_cmd \u003d ["},{"line_number":6824,"context_line":"            \"OVN_Southbound\", {"},{"line_number":6825,"context_line":"                \"op\": \"update\","},{"line_number":6826,"context_line":"                \"table\": \"Service_Monitor\","}],"source_content_type":"text/x-python","patch_set":4,"id":"d6487188_af0c5d8c","line":6823,"in_reply_to":"e3f6cdd1_ca5a460b","updated":"2025-06-17 09:01:47.000000000","message":"ack, I try to prepare something like:\n\n```\n    def test_force_status_offline_service_monitor(self):\n        row_uuid \u003d self.sb_api.db_create(\n            \u0027Service_Monitor\u0027,\n            ip\u003d\u002710.0.0.2\u0027,\n            logical_port\u003d\u0027fake-port\u0027,\n            src_ip\u003d\u002710.0.0.1\u0027,\n            status\u003d\u0027online\u0027,\n            protocol\u003d[\u0027tcp\u0027],\n        ).execute(check_error\u003dTrue)\n\n        class FakeRow:\n            pass\n\n        row \u003d FakeRow()\n        row.uuid \u003d row_uuid\n\n        helper \u003d ovn_helper.OvnProviderHelper()\n        helper.force_status_offline_service_monitor(row)\n\n        result \u003d self.sb_api.lookup(\u0027Service_Monitor\u0027, row_uuid)\n        self.assertEqual(result.status, \u0027offline\u0027)\n```\n\nBut at test is inhering from TestOVNFunctionalBase just those SB DB tables are loaded in the sb_api FDB, Chassis_Private, Chassis, Encap, Port_Binding, Datapath_Binding, SB_Global and creating a ovn sb connection in addition to the one that already exists seems like too much for just this one test — wdyt?","commit_id":"025a13bd2141cfd84117ba470c9cd5792c241322"}]}