)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":23567,"name":"Luis Tomas Bolivar","email":"ltomasbo@redhat.com","username":"ltomasbo"},"change_message_id":"ee98a04e7d9ee7c586fcbdb06ba2c61c946c5ccf","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"9f7baa52_ff598961","updated":"2023-05-11 06:15:10.000000000","message":"recheck","commit_id":"19c9291e736d655249629c9bf577631a83cb3548"}],"ovsdbapp/schema/ovn_northbound/api.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"9dde3677eebb3c22cb502ca51b22c5f1494b3261","unresolved":true,"context_lines":[{"line_number":753,"context_line":"        :param policy:    the policy used to make routing decisions"},{"line_number":754,"context_line":"        :type policy:     string, \u0027dst-ip\u0027 or \u0027src-ip\u0027"},{"line_number":755,"context_line":"        :param ecmp:      Enable ECMP support. If True adding routes with"},{"line_number":756,"context_line":"                          same ip prefix is allowed as long as the nexthop is"},{"line_number":757,"context_line":"                          different"},{"line_number":758,"context_line":"        :type ecmp:       boolean"},{"line_number":759,"context_line":"        :param may_exist: If True, don\u0027t fail if the route already exists"}],"source_content_type":"text/x-python","patch_set":1,"id":"86cf854f_9da21ed2","line":756,"range":{"start_line":756,"start_character":31,"end_line":756,"end_character":33},"updated":"2023-05-10 11:09:43.000000000","message":"s/ip/IP","commit_id":"6f7d76f2f74051021ab38c1797555f0e01778d1f"},{"author":{"_account_id":23567,"name":"Luis Tomas Bolivar","email":"ltomasbo@redhat.com","username":"ltomasbo"},"change_message_id":"cbac7287b33f7ee06f149792320f70a0325227fd","unresolved":false,"context_lines":[{"line_number":753,"context_line":"        :param policy:    the policy used to make routing decisions"},{"line_number":754,"context_line":"        :type policy:     string, \u0027dst-ip\u0027 or \u0027src-ip\u0027"},{"line_number":755,"context_line":"        :param ecmp:      Enable ECMP support. If True adding routes with"},{"line_number":756,"context_line":"                          same ip prefix is allowed as long as the nexthop is"},{"line_number":757,"context_line":"                          different"},{"line_number":758,"context_line":"        :type ecmp:       boolean"},{"line_number":759,"context_line":"        :param may_exist: If True, don\u0027t fail if the route already exists"}],"source_content_type":"text/x-python","patch_set":1,"id":"6364469c_18c1985a","line":756,"range":{"start_line":756,"start_character":31,"end_line":756,"end_character":33},"in_reply_to":"86cf854f_9da21ed2","updated":"2023-05-10 12:06:28.000000000","message":"Done","commit_id":"6f7d76f2f74051021ab38c1797555f0e01778d1f"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"a3a4122aa789a6f33681d4d6d34ab537efe1992b","unresolved":true,"context_lines":[{"line_number":735,"context_line":""},{"line_number":736,"context_line":"    @abc.abstractmethod"},{"line_number":737,"context_line":"    def lr_route_add(self, router, prefix, nexthop, port\u003dNone,"},{"line_number":738,"context_line":"                     policy\u003d\u0027dst-ip\u0027, ecmp\u003dFalse, may_exist\u003dFalse):"},{"line_number":739,"context_line":"        \"\"\"Add a route to \u0027router\u0027"},{"line_number":740,"context_line":""},{"line_number":741,"context_line":"        :param router:    The name or uuid of the router"}],"source_content_type":"text/x-python","patch_set":2,"id":"623e15e1_23897264","line":738,"updated":"2023-05-11 17:55:19.000000000","message":"I think it\u0027s a good rule of a thumb to always append parameter as the last one, in case someone uses positional parameters.","commit_id":"19c9291e736d655249629c9bf577631a83cb3548"},{"author":{"_account_id":23567,"name":"Luis Tomas Bolivar","email":"ltomasbo@redhat.com","username":"ltomasbo"},"change_message_id":"bbc93a456ff775e721390fa459118c5b2acb305c","unresolved":false,"context_lines":[{"line_number":735,"context_line":""},{"line_number":736,"context_line":"    @abc.abstractmethod"},{"line_number":737,"context_line":"    def lr_route_add(self, router, prefix, nexthop, port\u003dNone,"},{"line_number":738,"context_line":"                     policy\u003d\u0027dst-ip\u0027, ecmp\u003dFalse, may_exist\u003dFalse):"},{"line_number":739,"context_line":"        \"\"\"Add a route to \u0027router\u0027"},{"line_number":740,"context_line":""},{"line_number":741,"context_line":"        :param router:    The name or uuid of the router"}],"source_content_type":"text/x-python","patch_set":2,"id":"b583dd29_bd1ec677","line":738,"in_reply_to":"623e15e1_23897264","updated":"2023-05-16 07:01:16.000000000","message":"sure, I\u0027ll change it, I thought it was cleaner to have may_exists always as last parameter, but you are right, perhaps there is someone using the existing 3 optional parameters using positional","commit_id":"19c9291e736d655249629c9bf577631a83cb3548"}],"ovsdbapp/schema/ovn_northbound/commands.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"9630bd231f4189cdd25a6354093d3087638c3d11","unresolved":true,"context_lines":[{"line_number":1156,"context_line":"        lr \u003d self.api.lookup(\u0027Logical_Router\u0027, self.router)"},{"line_number":1157,"context_line":"        for route in lr.static_routes:"},{"line_number":1158,"context_line":"            if self.prefix \u003d\u003d route.ip_prefix:"},{"line_number":1159,"context_line":"                if self.ecmp and self.nexthop !\u003d route.nexthop:"},{"line_number":1160,"context_line":"                    continue"},{"line_number":1161,"context_line":"                if not self.may_exist:"},{"line_number":1162,"context_line":"                    msg \u003d \"Route %s already exists on router %s\" % ("}],"source_content_type":"text/x-python","patch_set":3,"id":"20a2b714_5ad72f4b","line":1159,"updated":"2023-05-24 13:41:23.000000000","message":"I\u0027m not really familiar with the ecmp handling, so I will defer to you on this. But when I look at ovn-nbctl.c (https://github.com/ovn-org/ovn/blob/4a1ca1e658f92ddfbd282d5a1bbf2affdb5e7f31/utilities/ovn-nbctl.c#L4535) it looks like in the case that ecmp is set, ovn-nbctl will not do any update, where this would do the updates if ecmp and self.nexthop \u003d\u003d route.nexthop. Is this difference intentional?","commit_id":"cc1ddf6a9e71c7a35be776e78a94af090f50c3f9"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"a22110c4cfe7af74b66051a92e3f033b8a8d86b7","unresolved":true,"context_lines":[{"line_number":1156,"context_line":"        lr \u003d self.api.lookup(\u0027Logical_Router\u0027, self.router)"},{"line_number":1157,"context_line":"        for route in lr.static_routes:"},{"line_number":1158,"context_line":"            if self.prefix \u003d\u003d route.ip_prefix:"},{"line_number":1159,"context_line":"                if self.ecmp and self.nexthop !\u003d route.nexthop:"},{"line_number":1160,"context_line":"                    continue"},{"line_number":1161,"context_line":"                if not self.may_exist:"},{"line_number":1162,"context_line":"                    msg \u003d \"Route %s already exists on router %s\" % ("}],"source_content_type":"text/x-python","patch_set":3,"id":"e082a0d4_b29590c7","line":1159,"in_reply_to":"0c7ffd83_164b7ff6","updated":"2023-05-24 14:07:45.000000000","message":"Ah, I see. Sounds reasonable to me. This looks like it is doing the same thing as nbctl (though the code is pretty different looking).","commit_id":"cc1ddf6a9e71c7a35be776e78a94af090f50c3f9"},{"author":{"_account_id":23567,"name":"Luis Tomas Bolivar","email":"ltomasbo@redhat.com","username":"ltomasbo"},"change_message_id":"bbab2c486fc1aa67ab1a8b645c637d89a333a160","unresolved":true,"context_lines":[{"line_number":1156,"context_line":"        lr \u003d self.api.lookup(\u0027Logical_Router\u0027, self.router)"},{"line_number":1157,"context_line":"        for route in lr.static_routes:"},{"line_number":1158,"context_line":"            if self.prefix \u003d\u003d route.ip_prefix:"},{"line_number":1159,"context_line":"                if self.ecmp and self.nexthop !\u003d route.nexthop:"},{"line_number":1160,"context_line":"                    continue"},{"line_number":1161,"context_line":"                if not self.may_exist:"},{"line_number":1162,"context_line":"                    msg \u003d \"Route %s already exists on router %s\" % ("}],"source_content_type":"text/x-python","patch_set":3,"id":"0c7ffd83_164b7ff6","line":1159,"in_reply_to":"20a2b714_5ad72f4b","updated":"2023-05-24 13:52:54.000000000","message":"The idea is not to update the existing route but adding an extra one, meaning you can have 2 different nexthops for the same ip_prefix.\n\nSo, the idea here (if I did not messed with it) is that if ecmp is enabled and the nexthop differs from the existing ones, I need to add a new entry (not update the existing one). By contrast, if the nexthop is teh same we just retrieve the existing one. Am I missing something?","commit_id":"cc1ddf6a9e71c7a35be776e78a94af090f50c3f9"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"b7a9cb91bf288d6ef0b894f9dadda4f87fc9260a","unresolved":false,"context_lines":[{"line_number":1156,"context_line":"        lr \u003d self.api.lookup(\u0027Logical_Router\u0027, self.router)"},{"line_number":1157,"context_line":"        for route in lr.static_routes:"},{"line_number":1158,"context_line":"            if self.prefix \u003d\u003d route.ip_prefix:"},{"line_number":1159,"context_line":"                if self.ecmp and self.nexthop !\u003d route.nexthop:"},{"line_number":1160,"context_line":"                    continue"},{"line_number":1161,"context_line":"                if not self.may_exist:"},{"line_number":1162,"context_line":"                    msg \u003d \"Route %s already exists on router %s\" % ("}],"source_content_type":"text/x-python","patch_set":3,"id":"10bf4dd4_71c0e6ca","line":1159,"in_reply_to":"e082a0d4_b29590c7","updated":"2023-05-24 14:10:46.000000000","message":".","commit_id":"cc1ddf6a9e71c7a35be776e78a94af090f50c3f9"}],"ovsdbapp/tests/functional/schema/ovn_northbound/test_impl_idl.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"9dde3677eebb3c22cb502ca51b22c5f1494b3261","unresolved":true,"context_lines":[{"line_number":995,"context_line":"    def test_lr_route_add_exists_ecmp(self):"},{"line_number":996,"context_line":"        router_name \u003d utils.get_rand_device_name()"},{"line_number":997,"context_line":"        self._lr_add_route(router_name)"},{"line_number":998,"context_line":"        self._lr_add_route(router_name, ecmp\u003dTrue)"},{"line_number":999,"context_line":""},{"line_number":1000,"context_line":"    def test_lr_route_add_discard(self):"},{"line_number":1001,"context_line":"        self._lr_add_route(nexthop\u003dconst.ROUTE_DISCARD)"}],"source_content_type":"text/x-python","patch_set":1,"id":"e0007693_7bf560ee","line":998,"range":{"start_line":998,"start_character":8,"end_line":998,"end_character":50},"updated":"2023-05-10 11:09:43.000000000","message":"This can\u0027t work: you are adding the same route again. That should fail in the check \"if self.ecmp and self.nexthop !\u003d route.nexthop\", because the nexthop is the same.","commit_id":"6f7d76f2f74051021ab38c1797555f0e01778d1f"},{"author":{"_account_id":23567,"name":"Luis Tomas Bolivar","email":"ltomasbo@redhat.com","username":"ltomasbo"},"change_message_id":"cbac7287b33f7ee06f149792320f70a0325227fd","unresolved":false,"context_lines":[{"line_number":995,"context_line":"    def test_lr_route_add_exists_ecmp(self):"},{"line_number":996,"context_line":"        router_name \u003d utils.get_rand_device_name()"},{"line_number":997,"context_line":"        self._lr_add_route(router_name)"},{"line_number":998,"context_line":"        self._lr_add_route(router_name, ecmp\u003dTrue)"},{"line_number":999,"context_line":""},{"line_number":1000,"context_line":"    def test_lr_route_add_discard(self):"},{"line_number":1001,"context_line":"        self._lr_add_route(nexthop\u003dconst.ROUTE_DISCARD)"}],"source_content_type":"text/x-python","patch_set":1,"id":"84495faf_22e0480d","line":998,"range":{"start_line":998,"start_character":8,"end_line":998,"end_character":50},"in_reply_to":"e0007693_7bf560ee","updated":"2023-05-10 12:06:28.000000000","message":"ups, right!forgot to add the different nexthop parameter","commit_id":"6f7d76f2f74051021ab38c1797555f0e01778d1f"}]}