)]}'
{"neutron/common/ovn/constants.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"b70345d8ffdca1e6908235b1636838145fbd171e","unresolved":false,"context_lines":[{"line_number":231,"context_line":""},{"line_number":232,"context_line":"# A special value reserved by maintenance task to indicate that the OVN"},{"line_number":233,"context_line":"# object exists but is to be considered to have stale revision number."},{"line_number":234,"context_line":"MAINTENANCE_STALE_REVISION_OBJECT \u003d {\u0027stale_object\u0027: True}"},{"line_number":235,"context_line":""},{"line_number":236,"context_line":"# The addresses field to set in the logical switch port which has a"},{"line_number":237,"context_line":"# peer router port (connecting to the logical router)."}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_b7a5fb49","line":234,"updated":"2020-07-23 14:39:20.000000000","message":"Maybe add something to maintenance.py that makes it clear that this is part of the official behavior? Since this relies on undocumented behavior in the _fix_create_update/_fix_delete. Maybe add a test that specifically tests passing this object? (not sure if you have one in a different patch set)","commit_id":"b2b1f898dc064bd3675e965146b311eb62e579b8"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"91d2fdb9be4c14e8a158e10d7a77d7b9ccab0853","unresolved":false,"context_lines":[{"line_number":231,"context_line":""},{"line_number":232,"context_line":"# A special value reserved by maintenance task to indicate that the OVN"},{"line_number":233,"context_line":"# object exists but is to be considered to have stale revision number."},{"line_number":234,"context_line":"MAINTENANCE_STALE_REVISION_OBJECT \u003d {\u0027stale_object\u0027: True}"},{"line_number":235,"context_line":""},{"line_number":236,"context_line":"# The addresses field to set in the logical switch port which has a"},{"line_number":237,"context_line":"# peer router port (connecting to the logical router)."}],"source_content_type":"text/x-python","patch_set":13,"id":"9f560f44_17d7f425","line":234,"in_reply_to":"bf51134e_91a600ab","updated":"2020-07-24 15:45:28.000000000","message":"After talking with Lucas and then looking at the code, I\u0027m now convinced that this is yet another case where \"less is more\". :^) Since we rely on an atomic transaction to change the revision number of the load balancer entries, it is not feasible to worry that the entries would ever have different revision numbers.\n\nEven if that ever happens, it would be better to fix that bug instead of adding defensive check overhead. We still have db_sync util as last resort, but in the interest of simplicity, there is no point in doing the extra check here.\n\nRemoving it. Thank you Lucas!!!","commit_id":"b2b1f898dc064bd3675e965146b311eb62e579b8"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"e6c4f0fd02298b6b270bb23c215d70b2ff59be6d","unresolved":false,"context_lines":[{"line_number":231,"context_line":""},{"line_number":232,"context_line":"# A special value reserved by maintenance task to indicate that the OVN"},{"line_number":233,"context_line":"# object exists but is to be considered to have stale revision number."},{"line_number":234,"context_line":"MAINTENANCE_STALE_REVISION_OBJECT \u003d {\u0027stale_object\u0027: True}"},{"line_number":235,"context_line":""},{"line_number":236,"context_line":"# The addresses field to set in the logical switch port which has a"},{"line_number":237,"context_line":"# peer router port (connecting to the logical router)."}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_91a600ab","line":234,"in_reply_to":"bf51134e_b7a5fb49","updated":"2020-07-23 18:45:52.000000000","message":"Good point. Let\u0027s get Lucas\u0027 blessings on this before adding the test.","commit_id":"b2b1f898dc064bd3675e965146b311eb62e579b8"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"003665dd2537ca61dc9612fabf1194b239dc585b","unresolved":false,"context_lines":[{"line_number":633,"context_line":"        result \u003d []"},{"line_number":634,"context_line":"        for ovn_obj in rc.execute(check_error\u003dTrue):"},{"line_number":635,"context_line":"            # Filter out router lbs that don\u0027t have fip id"},{"line_number":636,"context_line":"            ext_ids \u003d ovn_obj.get(\u0027external_ids\u0027, {})"},{"line_number":637,"context_line":"            if ext_ids.get(ovn_const.OVN_FIP_EXT_ID_KEY):"},{"line_number":638,"context_line":"                result.append(ovn_obj)"},{"line_number":639,"context_line":"        return result"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_ec6d6c32","line":636,"range":{"start_line":636,"start_character":22,"end_line":636,"end_character":53},"updated":"2020-07-15 21:05:40.000000000","message":"nit: Since \u0027external_ids\u0027 is defined in the schema, we should always have it in the result and it should always be a dict unless something is very broken. Also, instead of db_find, could do db_find_rows and just essentially do (I think):\n\n rc \u003d self.db_find_rows(...)\n return [obj for obj in rc.execute(check_error\u003dTrue)\n         if ovn_const.OVN_FIP_EXT_ID_KEY in obj.external_ids]\n\nof course the calling code would have to expect a list of rows instead of a list of dicts (unless db_find/obj[\u0027external_ids\u0027] above). Nothing wrong with being really defensive, just *should* be impossible to hit unless something really drastic changes.","commit_id":"bff91885e9a18825b7ce9eaa85fa2fa745c71e6c"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"52812ecee521035c77cd78c86b072c5988b88748","unresolved":false,"context_lines":[{"line_number":633,"context_line":"        result \u003d []"},{"line_number":634,"context_line":"        for ovn_obj in rc.execute(check_error\u003dTrue):"},{"line_number":635,"context_line":"            # Filter out router lbs that don\u0027t have fip id"},{"line_number":636,"context_line":"            ext_ids \u003d ovn_obj.get(\u0027external_ids\u0027, {})"},{"line_number":637,"context_line":"            if ext_ids.get(ovn_const.OVN_FIP_EXT_ID_KEY):"},{"line_number":638,"context_line":"                result.append(ovn_obj)"},{"line_number":639,"context_line":"        return result"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_4261bfd2","line":636,"range":{"start_line":636,"start_character":22,"end_line":636,"end_character":53},"in_reply_to":"bf51134e_ec6d6c32","updated":"2020-07-15 22:20:06.000000000","message":"Ack. How about keeping it \"dict\", but make it less defensive about \u0027external_ids\u0027? I will do that.","commit_id":"bff91885e9a18825b7ce9eaa85fa2fa745c71e6c"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"003665dd2537ca61dc9612fabf1194b239dc585b","unresolved":false,"context_lines":[{"line_number":638,"context_line":"                result.append(ovn_obj)"},{"line_number":639,"context_line":"        return result"},{"line_number":640,"context_line":""},{"line_number":641,"context_line":"    def get_floatingip_in_nat_or_lb(self, fip_id):"},{"line_number":642,"context_line":"        rc \u003d self.get_floatingip(fip_id)"},{"line_number":643,"context_line":"        if rc:"},{"line_number":644,"context_line":"            return rc"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_ec256cc3","line":641,"range":{"start_line":641,"start_character":8,"end_line":641,"end_character":35},"updated":"2020-07-15 21:05:40.000000000","message":"It\u0027s a little confusing having these functions defined in this patchset, but their use being in another patchset (and it looks like finding where it is ultimately called is tricky with the method being assigned to a dict).\n\nSo this method returns either {\u0027update_me\u0027: True} or if all of the revision numbers match, the first match?","commit_id":"bff91885e9a18825b7ce9eaa85fa2fa745c71e6c"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"52812ecee521035c77cd78c86b072c5988b88748","unresolved":false,"context_lines":[{"line_number":638,"context_line":"                result.append(ovn_obj)"},{"line_number":639,"context_line":"        return result"},{"line_number":640,"context_line":""},{"line_number":641,"context_line":"    def get_floatingip_in_nat_or_lb(self, fip_id):"},{"line_number":642,"context_line":"        rc \u003d self.get_floatingip(fip_id)"},{"line_number":643,"context_line":"        if rc:"},{"line_number":644,"context_line":"            return rc"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_c2a2effb","line":641,"range":{"start_line":641,"start_character":8,"end_line":641,"end_character":35},"in_reply_to":"bf51134e_ec256cc3","updated":"2020-07-15 22:20:06.000000000","message":"Right. Sorry for the confusion. I was trying to keep the file modified in only one of the patchsets and not a piece here and another part there.\n\nThis function is used as part of the maintenance task that ensures that the port forwarding has the proper revision number. It gets used here:\nhttps://review.opendev.org/#/c/741303/2/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py@160\n\nSince the caller expects one answer, the logic here ensures that the one answer that it returns is consistent about the revision number of all entries. If that is not the case, the returning of a dictionary that does not have OVN_REV_NUM_EXT_ID_KEY is enough to make maintenance task create/update the ovn entry. I will add some description of that in the code.","commit_id":"bff91885e9a18825b7ce9eaa85fa2fa745c71e6c"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"003665dd2537ca61dc9612fabf1194b239dc585b","unresolved":false,"context_lines":[{"line_number":652,"context_line":"        # the same, then an update is needed."},{"line_number":653,"context_line":"        prev_ovn_revision \u003d None"},{"line_number":654,"context_line":"        for ovn_obj in result:"},{"line_number":655,"context_line":"            ext_ids \u003d ovn_obj.get(\u0027external_ids\u0027, {})"},{"line_number":656,"context_line":"            ovn_revision \u003d ext_ids.get(ovn_const.OVN_REV_NUM_EXT_ID_KEY, \u0027-1\u0027)"},{"line_number":657,"context_line":"            if prev_ovn_revision and prev_ovn_revision !\u003d ovn_revision:"},{"line_number":658,"context_line":"                return {\u0027update_me\u0027: True}"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_8c9f9000","line":655,"range":{"start_line":655,"start_character":22,"end_line":655,"end_character":53},"updated":"2020-07-15 21:05:40.000000000","message":"similar comment as above.","commit_id":"bff91885e9a18825b7ce9eaa85fa2fa745c71e6c"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"52812ecee521035c77cd78c86b072c5988b88748","unresolved":false,"context_lines":[{"line_number":652,"context_line":"        # the same, then an update is needed."},{"line_number":653,"context_line":"        prev_ovn_revision \u003d None"},{"line_number":654,"context_line":"        for ovn_obj in result:"},{"line_number":655,"context_line":"            ext_ids \u003d ovn_obj.get(\u0027external_ids\u0027, {})"},{"line_number":656,"context_line":"            ovn_revision \u003d ext_ids.get(ovn_const.OVN_REV_NUM_EXT_ID_KEY, \u0027-1\u0027)"},{"line_number":657,"context_line":"            if prev_ovn_revision and prev_ovn_revision !\u003d ovn_revision:"},{"line_number":658,"context_line":"                return {\u0027update_me\u0027: True}"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_c77b2178","line":655,"range":{"start_line":655,"start_character":22,"end_line":655,"end_character":53},"in_reply_to":"bf51134e_8c9f9000","updated":"2020-07-15 22:20:06.000000000","message":"ack. Will fix this.","commit_id":"bff91885e9a18825b7ce9eaa85fa2fa745c71e6c"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"003665dd2537ca61dc9612fabf1194b239dc585b","unresolved":false,"context_lines":[{"line_number":657,"context_line":"            if prev_ovn_revision and prev_ovn_revision !\u003d ovn_revision:"},{"line_number":658,"context_line":"                return {\u0027update_me\u0027: True}"},{"line_number":659,"context_line":"            prev_ovn_revision \u003d ovn_revision"},{"line_number":660,"context_line":"        return result[0]"},{"line_number":661,"context_line":""},{"line_number":662,"context_line":"    def get_floatingip(self, fip_id):"},{"line_number":663,"context_line":"        # TODO(dalvarez): remove this check once the minimum OVS required"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_ac3894ed","line":660,"updated":"2020-07-15 21:05:40.000000000","message":"If we end up with empty results [], I think this will raise an IndexError. Not sure if the method using it catches that or not. :)","commit_id":"bff91885e9a18825b7ce9eaa85fa2fa745c71e6c"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"52812ecee521035c77cd78c86b072c5988b88748","unresolved":false,"context_lines":[{"line_number":657,"context_line":"            if prev_ovn_revision and prev_ovn_revision !\u003d ovn_revision:"},{"line_number":658,"context_line":"                return {\u0027update_me\u0027: True}"},{"line_number":659,"context_line":"            prev_ovn_revision \u003d ovn_revision"},{"line_number":660,"context_line":"        return result[0]"},{"line_number":661,"context_line":""},{"line_number":662,"context_line":"    def get_floatingip(self, fip_id):"},{"line_number":663,"context_line":"        # TODO(dalvarez): remove this check once the minimum OVS required"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_67885570","line":660,"in_reply_to":"bf51134e_ac3894ed","updated":"2020-07-15 22:20:06.000000000","message":"Oh, right. I check for that case at line 649. So if we make it this far there should be 1 or more entries in result.","commit_id":"bff91885e9a18825b7ce9eaa85fa2fa745c71e6c"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"5daab75b977af9edda95d71a3647442e61765e66","unresolved":false,"context_lines":[{"line_number":657,"context_line":"            if prev_ovn_revision and prev_ovn_revision !\u003d ovn_revision:"},{"line_number":658,"context_line":"                return {\u0027update_me\u0027: True}"},{"line_number":659,"context_line":"            prev_ovn_revision \u003d ovn_revision"},{"line_number":660,"context_line":"        return result[0]"},{"line_number":661,"context_line":""},{"line_number":662,"context_line":"    def get_floatingip(self, fip_id):"},{"line_number":663,"context_line":"        # TODO(dalvarez): remove this check once the minimum OVS required"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_a7e70de3","line":660,"in_reply_to":"bf51134e_ac3894ed","updated":"2020-07-15 21:41:31.000000000","message":"oops, as you mentioned when we chatted, i missed the if not result above. :)","commit_id":"bff91885e9a18825b7ce9eaa85fa2fa745c71e6c"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"9b541f9f8f2da69c6e679cad36955a25d1ff3c68","unresolved":false,"context_lines":[{"line_number":633,"context_line":"        result \u003d []"},{"line_number":634,"context_line":"        for ovn_obj in rc.execute(check_error\u003dTrue):"},{"line_number":635,"context_line":"            # Filter out router lbs that don\u0027t have fip id"},{"line_number":636,"context_line":"            ext_ids \u003d ovn_obj[\u0027external_ids\u0027]"},{"line_number":637,"context_line":"            if ext_ids.get(ovn_const.OVN_FIP_EXT_ID_KEY):"},{"line_number":638,"context_line":"                result.append(ovn_obj)"},{"line_number":639,"context_line":"        return result"},{"line_number":640,"context_line":""},{"line_number":641,"context_line":"    def get_floatingip_in_nat_or_lb(self, fip_id):"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_82373b4e","line":638,"range":{"start_line":636,"start_character":0,"end_line":638,"end_character":38},"updated":"2020-07-17 12:30:08.000000000","message":"What about using filter-lambda construction here? Wouldn\u0027t it be faster?","commit_id":"46ca528f3fa3c01510810c6c411f9be1c7628d04"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"9ee336d118e8c1d8792c32033c0dcea0f228aea0","unresolved":false,"context_lines":[{"line_number":633,"context_line":"        result \u003d []"},{"line_number":634,"context_line":"        for ovn_obj in rc.execute(check_error\u003dTrue):"},{"line_number":635,"context_line":"            # Filter out router lbs that don\u0027t have fip id"},{"line_number":636,"context_line":"            ext_ids \u003d ovn_obj[\u0027external_ids\u0027]"},{"line_number":637,"context_line":"            if ext_ids.get(ovn_const.OVN_FIP_EXT_ID_KEY):"},{"line_number":638,"context_line":"                result.append(ovn_obj)"},{"line_number":639,"context_line":"        return result"},{"line_number":640,"context_line":""},{"line_number":641,"context_line":"    def get_floatingip_in_nat_or_lb(self, fip_id):"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_03f674fa","line":638,"range":{"start_line":636,"start_character":0,"end_line":638,"end_character":38},"in_reply_to":"bf51134e_7328f7ed","updated":"2020-07-20 15:26:39.000000000","message":"Done","commit_id":"46ca528f3fa3c01510810c6c411f9be1c7628d04"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"9ac422386921e8907c72f9d6f6d1a5fc45220535","unresolved":false,"context_lines":[{"line_number":633,"context_line":"        result \u003d []"},{"line_number":634,"context_line":"        for ovn_obj in rc.execute(check_error\u003dTrue):"},{"line_number":635,"context_line":"            # Filter out router lbs that don\u0027t have fip id"},{"line_number":636,"context_line":"            ext_ids \u003d ovn_obj[\u0027external_ids\u0027]"},{"line_number":637,"context_line":"            if ext_ids.get(ovn_const.OVN_FIP_EXT_ID_KEY):"},{"line_number":638,"context_line":"                result.append(ovn_obj)"},{"line_number":639,"context_line":"        return result"},{"line_number":640,"context_line":""},{"line_number":641,"context_line":"    def get_floatingip_in_nat_or_lb(self, fip_id):"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_7328f7ed","line":638,"range":{"start_line":636,"start_character":0,"end_line":638,"end_character":38},"in_reply_to":"bf51134e_82373b4e","updated":"2020-07-17 14:51:54.000000000","message":"ack, will do.","commit_id":"46ca528f3fa3c01510810c6c411f9be1c7628d04"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"9b541f9f8f2da69c6e679cad36955a25d1ff3c68","unresolved":false,"context_lines":[{"line_number":643,"context_line":"        if rc:"},{"line_number":644,"context_line":"            return rc"},{"line_number":645,"context_line":""},{"line_number":646,"context_line":"        fip \u003d self.db_find(\u0027Load_Balancer\u0027, ("},{"line_number":647,"context_line":"            \u0027external_ids\u0027, \u0027\u003d\u0027, {ovn_const.OVN_FIP_EXT_ID_KEY: fip_id}))"},{"line_number":648,"context_line":"        result \u003d fip.execute(check_error\u003dTrue)"},{"line_number":649,"context_line":"        if not result:"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_221f4fb8","line":646,"updated":"2020-07-17 12:30:08.000000000","message":"Can we add here additional conditional to db_find, I mean:\n\n{ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY: PORT_FORWARDING_PLUGIN}\n\nLike it is in L631? It would be super-save for OVN Octavia provider","commit_id":"46ca528f3fa3c01510810c6c411f9be1c7628d04"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"9ac422386921e8907c72f9d6f6d1a5fc45220535","unresolved":false,"context_lines":[{"line_number":643,"context_line":"        if rc:"},{"line_number":644,"context_line":"            return rc"},{"line_number":645,"context_line":""},{"line_number":646,"context_line":"        fip \u003d self.db_find(\u0027Load_Balancer\u0027, ("},{"line_number":647,"context_line":"            \u0027external_ids\u0027, \u0027\u003d\u0027, {ovn_const.OVN_FIP_EXT_ID_KEY: fip_id}))"},{"line_number":648,"context_line":"        result \u003d fip.execute(check_error\u003dTrue)"},{"line_number":649,"context_line":"        if not result:"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_d3f3c39e","line":646,"in_reply_to":"bf51134e_221f4fb8","updated":"2020-07-17 14:51:54.000000000","message":"yes, good catch!","commit_id":"46ca528f3fa3c01510810c6c411f9be1c7628d04"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"9ee336d118e8c1d8792c32033c0dcea0f228aea0","unresolved":false,"context_lines":[{"line_number":643,"context_line":"        if rc:"},{"line_number":644,"context_line":"            return rc"},{"line_number":645,"context_line":""},{"line_number":646,"context_line":"        fip \u003d self.db_find(\u0027Load_Balancer\u0027, ("},{"line_number":647,"context_line":"            \u0027external_ids\u0027, \u0027\u003d\u0027, {ovn_const.OVN_FIP_EXT_ID_KEY: fip_id}))"},{"line_number":648,"context_line":"        result \u003d fip.execute(check_error\u003dTrue)"},{"line_number":649,"context_line":"        if not result:"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_a3e4e83d","line":646,"in_reply_to":"bf51134e_d3f3c39e","updated":"2020-07-20 15:26:39.000000000","message":"Done","commit_id":"46ca528f3fa3c01510810c6c411f9be1c7628d04"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"8cbe9ad6ba4ff4543cdfdb3cac83d65a4a2ff834","unresolved":false,"context_lines":[{"line_number":33,"context_line":"from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf as cfg"},{"line_number":34,"context_line":"from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import commands as cmd"},{"line_number":35,"context_line":"from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovsdb_monitor"},{"line_number":36,"context_line":"from neutron.services.portforwarding.constants import PORT_FORWARDING_PLUGIN"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"LOG \u003d log.getLogger(__name__)"}],"source_content_type":"text/x-python","patch_set":7,"id":"bf51134e_032b1430","line":36,"updated":"2020-07-20 15:38:53.000000000","message":"You should import the whole constants module","commit_id":"e4a77163cabcaeba9350b393d6169dd6688aff14"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"53580b6b40d9f6aff99af1ae253fd942811deaf7","unresolved":false,"context_lines":[{"line_number":33,"context_line":"from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf as cfg"},{"line_number":34,"context_line":"from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import commands as cmd"},{"line_number":35,"context_line":"from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovsdb_monitor"},{"line_number":36,"context_line":"from neutron.services.portforwarding.constants import PORT_FORWARDING_PLUGIN"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"LOG \u003d log.getLogger(__name__)"}],"source_content_type":"text/x-python","patch_set":7,"id":"bf51134e_c3c91c26","line":36,"in_reply_to":"bf51134e_032b1430","updated":"2020-07-20 16:01:35.000000000","message":"ack!","commit_id":"e4a77163cabcaeba9350b393d6169dd6688aff14"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"8cbe9ad6ba4ff4543cdfdb3cac83d65a4a2ff834","unresolved":false,"context_lines":[{"line_number":634,"context_line":"                if ovn_const.OVN_FIP_EXT_ID_KEY in ovn_obj[\u0027external_ids\u0027]]"},{"line_number":635,"context_line":""},{"line_number":636,"context_line":"    def get_floatingip_in_nat_or_lb(self, fip_id):"},{"line_number":637,"context_line":"        rc \u003d self.get_floatingip(fip_id)"},{"line_number":638,"context_line":"        if rc:"},{"line_number":639,"context_line":"            return rc"},{"line_number":640,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"bf51134e_a3198823","line":637,"range":{"start_line":637,"start_character":8,"end_line":637,"end_character":10},"updated":"2020-07-20 15:38:53.000000000","message":"What does this stand for? Maybe fip would be better?","commit_id":"e4a77163cabcaeba9350b393d6169dd6688aff14"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"53580b6b40d9f6aff99af1ae253fd942811deaf7","unresolved":false,"context_lines":[{"line_number":634,"context_line":"                if ovn_const.OVN_FIP_EXT_ID_KEY in ovn_obj[\u0027external_ids\u0027]]"},{"line_number":635,"context_line":""},{"line_number":636,"context_line":"    def get_floatingip_in_nat_or_lb(self, fip_id):"},{"line_number":637,"context_line":"        rc \u003d self.get_floatingip(fip_id)"},{"line_number":638,"context_line":"        if rc:"},{"line_number":639,"context_line":"            return rc"},{"line_number":640,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"bf51134e_43834caa","line":637,"range":{"start_line":637,"start_character":8,"end_line":637,"end_character":10},"in_reply_to":"bf51134e_a3198823","updated":"2020-07-20 16:01:35.000000000","message":"good point. this was a copy and paste from\n_get_logical_router_port_gateway_chassis but I think that is\na lame excuse. ;) Will fix.","commit_id":"e4a77163cabcaeba9350b393d6169dd6688aff14"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"8cbe9ad6ba4ff4543cdfdb3cac83d65a4a2ff834","unresolved":false,"context_lines":[{"line_number":648,"context_line":"        # Check revision numbers of entries found. If they are not"},{"line_number":649,"context_line":"        # the same, then an update is needed."},{"line_number":650,"context_line":"        prev_ovn_revision \u003d None"},{"line_number":651,"context_line":"        for ovn_obj in result:"},{"line_number":652,"context_line":"            ext_ids \u003d ovn_obj[\u0027external_ids\u0027]"},{"line_number":653,"context_line":"            ovn_revision \u003d ext_ids.get(ovn_const.OVN_REV_NUM_EXT_ID_KEY, \u0027-1\u0027)"},{"line_number":654,"context_line":"            if prev_ovn_revision and prev_ovn_revision !\u003d ovn_revision:"}],"source_content_type":"text/x-python","patch_set":7,"id":"bf51134e_63e4900f","line":651,"range":{"start_line":651,"start_character":12,"end_line":651,"end_character":19},"updated":"2020-07-20 15:38:53.000000000","message":"fip ?","commit_id":"e4a77163cabcaeba9350b393d6169dd6688aff14"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"53580b6b40d9f6aff99af1ae253fd942811deaf7","unresolved":false,"context_lines":[{"line_number":648,"context_line":"        # Check revision numbers of entries found. If they are not"},{"line_number":649,"context_line":"        # the same, then an update is needed."},{"line_number":650,"context_line":"        prev_ovn_revision \u003d None"},{"line_number":651,"context_line":"        for ovn_obj in result:"},{"line_number":652,"context_line":"            ext_ids \u003d ovn_obj[\u0027external_ids\u0027]"},{"line_number":653,"context_line":"            ovn_revision \u003d ext_ids.get(ovn_const.OVN_REV_NUM_EXT_ID_KEY, \u0027-1\u0027)"},{"line_number":654,"context_line":"            if prev_ovn_revision and prev_ovn_revision !\u003d ovn_revision:"}],"source_content_type":"text/x-python","patch_set":7,"id":"bf51134e_e3bb8072","line":651,"range":{"start_line":651,"start_character":12,"end_line":651,"end_character":19},"in_reply_to":"bf51134e_63e4900f","updated":"2020-07-20 16:01:35.000000000","message":"actually it is ovn_lb. will change that too. ;)","commit_id":"e4a77163cabcaeba9350b393d6169dd6688aff14"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"8cbe9ad6ba4ff4543cdfdb3cac83d65a4a2ff834","unresolved":false,"context_lines":[{"line_number":651,"context_line":"        for ovn_obj in result:"},{"line_number":652,"context_line":"            ext_ids \u003d ovn_obj[\u0027external_ids\u0027]"},{"line_number":653,"context_line":"            ovn_revision \u003d ext_ids.get(ovn_const.OVN_REV_NUM_EXT_ID_KEY, \u0027-1\u0027)"},{"line_number":654,"context_line":"            if prev_ovn_revision and prev_ovn_revision !\u003d ovn_revision:"},{"line_number":655,"context_line":"                # If we make it here, we found 2 entries for the same fip"},{"line_number":656,"context_line":"                # that have mismatching OVN_REV_NUM_EXT_ID_KEY values."},{"line_number":657,"context_line":"                # Since caller expects only one entry, the best way to"}],"source_content_type":"text/x-python","patch_set":7,"id":"bf51134e_6392d05a","line":654,"range":{"start_line":654,"start_character":15,"end_line":654,"end_character":32},"updated":"2020-07-20 15:38:53.000000000","message":"can REV_NUM be a None? It seems you don\u0027t need this condition","commit_id":"e4a77163cabcaeba9350b393d6169dd6688aff14"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"53580b6b40d9f6aff99af1ae253fd942811deaf7","unresolved":false,"context_lines":[{"line_number":651,"context_line":"        for ovn_obj in result:"},{"line_number":652,"context_line":"            ext_ids \u003d ovn_obj[\u0027external_ids\u0027]"},{"line_number":653,"context_line":"            ovn_revision \u003d ext_ids.get(ovn_const.OVN_REV_NUM_EXT_ID_KEY, \u0027-1\u0027)"},{"line_number":654,"context_line":"            if prev_ovn_revision and prev_ovn_revision !\u003d ovn_revision:"},{"line_number":655,"context_line":"                # If we make it here, we found 2 entries for the same fip"},{"line_number":656,"context_line":"                # that have mismatching OVN_REV_NUM_EXT_ID_KEY values."},{"line_number":657,"context_line":"                # Since caller expects only one entry, the best way to"}],"source_content_type":"text/x-python","patch_set":7,"id":"bf51134e_63af702b","line":654,"range":{"start_line":654,"start_character":15,"end_line":654,"end_character":32},"in_reply_to":"bf51134e_6392d05a","updated":"2020-07-20 16:01:35.000000000","message":"yeah, it can for the very first iteration. It is set on line 650 above. Let me see if I can dome something better here.","commit_id":"e4a77163cabcaeba9350b393d6169dd6688aff14"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"8cbe9ad6ba4ff4543cdfdb3cac83d65a4a2ff834","unresolved":false,"context_lines":[{"line_number":658,"context_line":"                # handle this is by returning a modified entry that has"},{"line_number":659,"context_line":"                # no \u0027external_ids\u0027. Returning None or {} indicates that"},{"line_number":660,"context_line":"                # no entries were found, which is not the case here."},{"line_number":661,"context_line":"                fun_rm_rev \u003d lambda obj: {"},{"line_number":662,"context_line":"                    key1: fun_rm_rev(val1) if isinstance(val1, dict) else val1"},{"line_number":663,"context_line":"                    for key1, val1 in obj.items()"},{"line_number":664,"context_line":"                    if key1 !\u003d ovn_const.OVN_REV_NUM_EXT_ID_KEY}"}],"source_content_type":"text/x-python","patch_set":7,"id":"bf51134e_83c44461","line":661,"range":{"start_line":661,"start_character":29,"end_line":661,"end_character":39},"updated":"2020-07-20 15:38:53.000000000","message":"Do you need this lambda object? They are discouraged https://www.python.org/dev/peps/pep-0008/#programming-recommendations\n\nand also it seems you don\u0027t need it to be a callable at all","commit_id":"e4a77163cabcaeba9350b393d6169dd6688aff14"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"53580b6b40d9f6aff99af1ae253fd942811deaf7","unresolved":false,"context_lines":[{"line_number":658,"context_line":"                # handle this is by returning a modified entry that has"},{"line_number":659,"context_line":"                # no \u0027external_ids\u0027. Returning None or {} indicates that"},{"line_number":660,"context_line":"                # no entries were found, which is not the case here."},{"line_number":661,"context_line":"                fun_rm_rev \u003d lambda obj: {"},{"line_number":662,"context_line":"                    key1: fun_rm_rev(val1) if isinstance(val1, dict) else val1"},{"line_number":663,"context_line":"                    for key1, val1 in obj.items()"},{"line_number":664,"context_line":"                    if key1 !\u003d ovn_const.OVN_REV_NUM_EXT_ID_KEY}"}],"source_content_type":"text/x-python","patch_set":7,"id":"bf51134e_43718cde","line":661,"range":{"start_line":661,"start_character":29,"end_line":661,"end_character":39},"in_reply_to":"bf51134e_83c44461","updated":"2020-07-20 16:01:35.000000000","message":"hmm... I see. I was hoping not to make this too hard to read, but you are right about making it non-callable. Will change.","commit_id":"e4a77163cabcaeba9350b393d6169dd6688aff14"},{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"5214810c9dd0a81fabd13437f4db6596af022d33","unresolved":false,"context_lines":[{"line_number":636,"context_line":""},{"line_number":637,"context_line":"    @staticmethod"},{"line_number":638,"context_line":"    def _filter_out_revision_number(dict_obj):"},{"line_number":639,"context_line":"        # Recursively iterate a dictionary object, removing \u0027revision_number\u0027"},{"line_number":640,"context_line":"        return {"},{"line_number":641,"context_line":"            key: OvsdbNbOvnIdl._filter_out_revision_number(val)"},{"line_number":642,"context_line":"            if isinstance(val, dict) else val"}],"source_content_type":"text/x-python","patch_set":11,"id":"bf51134e_00bbf8c3","line":639,"range":{"start_line":639,"start_character":9,"end_line":639,"end_character":49},"updated":"2020-07-21 13:46:17.000000000","message":"++ :)","commit_id":"177900c4a5f9764dcd428b4fd518627def58d939"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"e022f89a2a914cc43d8e1247dabbe23a1c49dae9","unresolved":false,"context_lines":[{"line_number":636,"context_line":""},{"line_number":637,"context_line":"    @staticmethod"},{"line_number":638,"context_line":"    def _filter_out_revision_number(dict_obj):"},{"line_number":639,"context_line":"        # Recursively iterate a dictionary object, removing \u0027revision_number\u0027"},{"line_number":640,"context_line":"        return {"},{"line_number":641,"context_line":"            key: OvsdbNbOvnIdl._filter_out_revision_number(val)"},{"line_number":642,"context_line":"            if isinstance(val, dict) else val"}],"source_content_type":"text/x-python","patch_set":11,"id":"bf51134e_d976fa3f","line":639,"range":{"start_line":639,"start_character":9,"end_line":639,"end_character":49},"in_reply_to":"bf51134e_00bbf8c3","updated":"2020-07-22 18:09:34.000000000","message":"The following is probably overthinking/overengineering/premature optimization, so feel free to ignore. But I think we could avoid iterating over the keys/creating a new dict/recursion by doing something like:\n\n    class RevisionlessDictProxy(MutableMapping):\n        def __init__(self, d):\n            self._d \u003d d\n\n        def __getitem__(self, key):\n            if key \u003d\u003d \u0027revision\u0027:\n                raise KeyError(key)\n            item \u003d self._d[key]\n            if isinstance(item, MutableMapping):\n                return RevisionlessDictProxy(self._d[key])\n            return self._d[key]\n\n        def __setitem__(self, key, value):\n            self._d[key] \u003d value\n\n        def __delitem__(self, key):\n            del self._d[key]\n\n        def __iter__(self):\n            return iter(self._d)\n\n        def __len__(self):\n            return len(self._d)\n\n\nwhich seems to work and also be faster with timeit. But again, maybe not worth it.","commit_id":"177900c4a5f9764dcd428b4fd518627def58d939"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"e42d94f034c3523147d66e9e06f306327541e3e8","unresolved":false,"context_lines":[{"line_number":636,"context_line":""},{"line_number":637,"context_line":"    @staticmethod"},{"line_number":638,"context_line":"    def _filter_out_revision_number(dict_obj):"},{"line_number":639,"context_line":"        # Recursively iterate a dictionary object, removing \u0027revision_number\u0027"},{"line_number":640,"context_line":"        return {"},{"line_number":641,"context_line":"            key: OvsdbNbOvnIdl._filter_out_revision_number(val)"},{"line_number":642,"context_line":"            if isinstance(val, dict) else val"}],"source_content_type":"text/x-python","patch_set":11,"id":"bf51134e_0f42e4ab","line":639,"range":{"start_line":639,"start_character":9,"end_line":639,"end_character":49},"in_reply_to":"bf51134e_af265867","updated":"2020-07-22 19:58:24.000000000","message":"Yes! Keep it simple is always my preferred choice. Let me revisit this and see if this does the job.","commit_id":"177900c4a5f9764dcd428b4fd518627def58d939"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"d1796e26ed051aa1143c2f0c1b7033812c2ffaed","unresolved":false,"context_lines":[{"line_number":636,"context_line":""},{"line_number":637,"context_line":"    @staticmethod"},{"line_number":638,"context_line":"    def _filter_out_revision_number(dict_obj):"},{"line_number":639,"context_line":"        # Recursively iterate a dictionary object, removing \u0027revision_number\u0027"},{"line_number":640,"context_line":"        return {"},{"line_number":641,"context_line":"            key: OvsdbNbOvnIdl._filter_out_revision_number(val)"},{"line_number":642,"context_line":"            if isinstance(val, dict) else val"}],"source_content_type":"text/x-python","patch_set":11,"id":"bf51134e_af265867","line":639,"range":{"start_line":639,"start_character":9,"end_line":639,"end_character":49},"in_reply_to":"bf51134e_d976fa3f","updated":"2020-07-22 19:38:33.000000000","message":"After chatting for a while w/ Flavio, I see that the maintenance code just needs a response that is \"something that does not evaluate to False\" in _fix_create_update/_fix_delete. Maybe if we just have a constant value in maintenance.py that is essentially\n\n OVN_GET_CONTINUE\u003d{\u0027keep\u0027: \u0027going\u0027}\n\nthat could be returned? Something that doesn\u0027t have overhead. An Exception we could raise would be nice, but handling it in the maintenance code looks like it\u0027d be a bit ugly.","commit_id":"177900c4a5f9764dcd428b4fd518627def58d939"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"8baa5fe99a4a64c1e014ce64b9465327b5c34c05","unresolved":false,"context_lines":[{"line_number":636,"context_line":"            {ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:"},{"line_number":637,"context_line":"                pf_const.PORT_FORWARDING_PLUGIN,"},{"line_number":638,"context_line":"             ovn_const.OVN_FIP_EXT_ID_KEY: fip_id})).execute(check_error\u003dTrue)"},{"line_number":639,"context_line":"        return result[0] if result else None"},{"line_number":640,"context_line":""},{"line_number":641,"context_line":"    def get_floatingip(self, fip_id):"},{"line_number":642,"context_line":"        # TODO(dalvarez): remove this check once the minimum OVS required"}],"source_content_type":"text/x-python","patch_set":15,"id":"9f560f44_6fcbe71d","line":639,"updated":"2020-07-27 10:08:36.000000000","message":"These are public methods, shouldn\u0027t they also be part of the api.py ?\n\nAlso would be nice to have some unit-tests for it.","commit_id":"4d2b9dfda45813a5e88cea330b174649e5769d16"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"9ab272fc08cb756b70b0c5eb1fd421166b59a029","unresolved":false,"context_lines":[{"line_number":636,"context_line":"            {ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:"},{"line_number":637,"context_line":"                pf_const.PORT_FORWARDING_PLUGIN,"},{"line_number":638,"context_line":"             ovn_const.OVN_FIP_EXT_ID_KEY: fip_id})).execute(check_error\u003dTrue)"},{"line_number":639,"context_line":"        return result[0] if result else None"},{"line_number":640,"context_line":""},{"line_number":641,"context_line":"    def get_floatingip(self, fip_id):"},{"line_number":642,"context_line":"        # TODO(dalvarez): remove this check once the minimum OVS required"}],"source_content_type":"text/x-python","patch_set":15,"id":"9f560f44_fc82014f","line":639,"in_reply_to":"9f560f44_6fcbe71d","updated":"2020-07-27 23:24:01.000000000","message":"Done","commit_id":"4d2b9dfda45813a5e88cea330b174649e5769d16"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"b0eda7f732840aaa4db876062466937ada848843","unresolved":false,"context_lines":[{"line_number":636,"context_line":"            {ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:"},{"line_number":637,"context_line":"                pf_const.PORT_FORWARDING_PLUGIN,"},{"line_number":638,"context_line":"             ovn_const.OVN_FIP_EXT_ID_KEY: fip_id})).execute(check_error\u003dTrue)"},{"line_number":639,"context_line":"        return result[0] if result else None"},{"line_number":640,"context_line":""},{"line_number":641,"context_line":"    def get_floatingip(self, fip_id):"},{"line_number":642,"context_line":"        # TODO(dalvarez): remove this check once the minimum OVS required"}],"source_content_type":"text/x-python","patch_set":15,"id":"9f560f44_e37270e3","line":639,"in_reply_to":"9f560f44_6fcbe71d","updated":"2020-07-27 12:39:11.000000000","message":"hm.... will check about api.py; did not think there was a need for it somehow.\n\nI have tests that exercise this, but it got broken up\nwith https://review.opendev.org/#/c/741303/  I will move this there.","commit_id":"4d2b9dfda45813a5e88cea330b174649e5769d16"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"676157f59d2f3a09f0e4da70804ffe334c739cd4","unresolved":false,"context_lines":[{"line_number":618,"context_line":""},{"line_number":619,"context_line":"        return (ls, None)"},{"line_number":620,"context_line":""},{"line_number":621,"context_line":"    def get_router_floatingip_lbs(self, lrouter_name):"},{"line_number":622,"context_line":"        rc \u003d self.db_find_rows(\u0027Load_Balancer\u0027, ("},{"line_number":623,"context_line":"            \u0027external_ids\u0027, \u0027\u003d\u0027,"},{"line_number":624,"context_line":"            {ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:"}],"source_content_type":"text/x-python","patch_set":20,"id":"9f560f44_82270913","line":621,"updated":"2020-07-29 08:16:53.000000000","message":"These two methods are great candidates for functional tests","commit_id":"597bb0d1877f92f9300d7d337bb9920ac82cab9a"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"1da4fa3deede3d2a2e5e92c701a96e02b9e28681","unresolved":false,"context_lines":[{"line_number":618,"context_line":""},{"line_number":619,"context_line":"        return (ls, None)"},{"line_number":620,"context_line":""},{"line_number":621,"context_line":"    def get_router_floatingip_lbs(self, lrouter_name):"},{"line_number":622,"context_line":"        rc \u003d self.db_find_rows(\u0027Load_Balancer\u0027, ("},{"line_number":623,"context_line":"            \u0027external_ids\u0027, \u0027\u003d\u0027,"},{"line_number":624,"context_line":"            {ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY:"}],"source_content_type":"text/x-python","patch_set":20,"id":"9f560f44_ff401738","line":621,"in_reply_to":"9f560f44_82270913","updated":"2020-07-29 09:39:50.000000000","message":"Will do. TY Kuba!","commit_id":"597bb0d1877f92f9300d7d337bb9920ac82cab9a"}]}