)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"76247bc3b1eb4fb8cc4f1cda1fc65845a07c97db","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Lucas Alvares Gomes \u003clucasagomes@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2021-10-13 11:57:25 +0100"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"[OVN] Allow for tuning the use of MAC_Binding"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"In order to avoid having a MAC_Binding table explosion and helping"},{"line_number":10,"context_line":"lowering the memory footprint when using ML2/OVN this patch is"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"c482d1ee_ee583d62","line":7,"updated":"2021-10-13 16:02:09.000000000","message":"now that you enable both by default, this headline doesn\u0027t reflect the effect. It suggests new tuning knobs are introduced but now that they will be in effect by default.","commit_id":"12890182d9b0a96239065e83e24b5748b711b224"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"f8943803d92821daefb8f7fcc7a7a62eacb9c866","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Lucas Alvares Gomes \u003clucasagomes@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2021-10-13 11:57:25 +0100"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"[OVN] Allow for tuning the use of MAC_Binding"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"In order to avoid having a MAC_Binding table explosion and helping"},{"line_number":10,"context_line":"lowering the memory footprint when using ML2/OVN this patch is"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"dcd153cb_3ca3b78b","line":7,"in_reply_to":"c482d1ee_ee583d62","updated":"2021-10-13 16:02:37.000000000","message":"s/but now/but not","commit_id":"12890182d9b0a96239065e83e24b5748b711b224"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"72680f38f64477ce09cbaac7845598ad98a0aacd","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Lucas Alvares Gomes \u003clucasagomes@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2021-10-13 11:57:25 +0100"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"[OVN] Allow for tuning the use of MAC_Binding"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"In order to avoid having a MAC_Binding table explosion and helping"},{"line_number":10,"context_line":"lowering the memory footprint when using ML2/OVN this patch is"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"31ebc093_ebe5fe42","line":7,"in_reply_to":"dcd153cb_3ca3b78b","updated":"2021-10-14 08:26:09.000000000","message":"Indeed, lemme change it.","commit_id":"12890182d9b0a96239065e83e24b5748b711b224"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"c29ff11ea29f2dd20d6bbb0c0edd4b114bba33ac","unresolved":true,"context_lines":[{"line_number":11,"context_line":"introducing two new configuration options that allows for tuning how"},{"line_number":12,"context_line":"OVN uses the MAC_Binding table:"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"* router_always_learn_from_arp_request: This option is a passthru for"},{"line_number":15,"context_line":"  the \"always_learn_from_arp_request\" option for the Logical Router"},{"line_number":16,"context_line":"  in OVN. This option controls the behavior of the OVN routers when"},{"line_number":17,"context_line":"  handling IPv4 ARP requests or IPv6 ND-NS packets."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"* dynamic_neigh_routers: This option is a passthru for"},{"line_number":20,"context_line":"  the \"dynamic_neigh_routers\" option for the Logical Router in"},{"line_number":21,"context_line":"  OVN. This option tells the OVN router to resolve neighbor routers"},{"line_number":22,"context_line":"  MAC addresses only by dynamic ARP/ND whe set to True."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"For more information on these option for core OVN please refer to:"},{"line_number":25,"context_line":"https://www.ovn.org/support/dist-docs/ovn-nb.5.html"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"c3cacff3_baf0a73b","line":22,"range":{"start_line":14,"start_character":0,"end_line":22,"end_character":55},"updated":"2021-10-14 09:05:59.000000000","message":"I would probably add some info (maybe in a release note?) explaining what\u0027s the impact of each option.\n\nIIUC:\n\nrouter_always_learn_from_arp_request -\u003e by setting this to False we\u0027ll avoid learning from ARP replies observed in the network. Only ARP requests sent by OVN will generate a MAC_Binding entry. For large broadcasts domains, this will avoid learning a large number of MAC addresses that are likely never going to be used by the overlay; hence reducing the size of the SB database and the churn.\n\n\ndynamic_neigh_routers -\u003e by setting this to True, we avoid pre-populating flows for router to router communication, hence reducing the number of logical flows (and SB database size) and the openflows installed on each hypervisor. This will have a great impact in the OpenStack case where tenants typically connect their routers to the provider network and we\u0027ll save a lot of flows. On demand, OVN will ARP router ports as needed and learn their MAC addresses through the MAC_Binding mechanism.\nIn the worst case scenario where all routers need to talk to all routers, we\u0027d end up with the same case and we\u0027ll have as many MAC_Binding entries as LFlows we had when this option is set to False.","commit_id":"12890182d9b0a96239065e83e24b5748b711b224"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e7edc3b0a92d61e203d7cef91d52c18baf966812","unresolved":true,"context_lines":[{"line_number":18,"context_line":"  of ML2/OVN."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"* dynamic_neigh_routers: By setting this to True we avoid"},{"line_number":21,"context_line":"  pre-populating flows for router to router communication, reduding"},{"line_number":22,"context_line":"  the number of flows, DB size and memory footprint of ML2/OVN."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"For more information on these option for core OVN please refer to:"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"7468224a_68fccf8c","line":21,"updated":"2021-10-14 14:01:52.000000000","message":"reducing","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"f5a68e54b039600d8b125b5c2ece003763cf1873","unresolved":false,"context_lines":[{"line_number":18,"context_line":"  of ML2/OVN."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"* dynamic_neigh_routers: By setting this to True we avoid"},{"line_number":21,"context_line":"  pre-populating flows for router to router communication, reduding"},{"line_number":22,"context_line":"  the number of flows, DB size and memory footprint of ML2/OVN."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"For more information on these option for core OVN please refer to:"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":10,"id":"9788bc72_c2cd5853","line":21,"in_reply_to":"7468224a_68fccf8c","updated":"2021-10-14 15:26:02.000000000","message":"Done","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"becdfb83b6180765895e55e4f794e55c46d0797f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"7e71812b_368d4f54","updated":"2021-10-12 14:55:42.000000000","message":"There\u0027s a similar option that we should also allow operators to tune in core OVN","commit_id":"f83e9ec2789ddfb9abb91dff247b87a7609c41bb"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"9f40e8d2d623fb1a058d4ccdb9b5c8d7e5b8d89e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"e32a66b9_740cf95e","updated":"2021-10-13 08:09:52.000000000","message":"Thanks for the review Ihar, good point on the default. I was just being conservative with it but I don\u0027t think there\u0027s any drawbacks on having it to False.","commit_id":"8c3d5da02a391b6d27cb8009a6d2545ef36091df"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"8f7076049b026a676a6a549f4f98b78bb62df9ca","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"54f15430_5da39afd","updated":"2021-10-13 08:03:38.000000000","message":"recheck","commit_id":"8c3d5da02a391b6d27cb8009a6d2545ef36091df"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"0feedc3ac99734518bb6da5d6a9c1d86fac520ee","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"425201c1_134e35ec","updated":"2021-10-15 10:30:07.000000000","message":"Awesome, the only thing is if we want to include it in the release notes somehow. I think it\u0027s a great improvement in terms of perf \u0026 scale","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e7edc3b0a92d61e203d7cef91d52c18baf966812","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"79f5fc78_045d2546","updated":"2021-10-14 14:01:52.000000000","message":"I am confused. Maybe a comment explaining the intention will help me understand.","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"}],"neutron/conf/plugins/ml2/drivers/ovn/ovn_conf.py":[{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"9ca761cabd7177dfe25bd81f76ec396c8cdbdf44","unresolved":true,"context_lines":[{"line_number":198,"context_line":"                       \u0027ovs-appctl -t ovs-vswitchd dpif/show-dp-features \u0027"},{"line_number":199,"context_line":"                       \u0027br-int | grep \"Check pkt length action\".\u0027)),"},{"line_number":200,"context_line":"    cfg.BoolOpt(\u0027router_always_learn_from_arp_request\u0027,"},{"line_number":201,"context_line":"                default\u003dTrue,"},{"line_number":202,"context_line":"                help\u003d_(\u0027This option controls the behavior of the OVN routers \u0027"},{"line_number":203,"context_line":"                       \u0027when handling IPv4 ARP requests or IPv6 ND-NS \u0027"},{"line_number":204,"context_line":"                       \u0027packets. This option is a passthru to set the \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"72b2a3dc_46fc3f65","line":201,"updated":"2021-10-12 16:26:57.000000000","message":"Perhaps a rationale on why these defaults were chosen? do we have a reason not to enable these by default? We definitely have numbers to suggest using these in large deployments. Do we have something to prove that this is bad for smaller deployments?","commit_id":"8c3d5da02a391b6d27cb8009a6d2545ef36091df"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"9f40e8d2d623fb1a058d4ccdb9b5c8d7e5b8d89e","unresolved":true,"context_lines":[{"line_number":198,"context_line":"                       \u0027ovs-appctl -t ovs-vswitchd dpif/show-dp-features \u0027"},{"line_number":199,"context_line":"                       \u0027br-int | grep \"Check pkt length action\".\u0027)),"},{"line_number":200,"context_line":"    cfg.BoolOpt(\u0027router_always_learn_from_arp_request\u0027,"},{"line_number":201,"context_line":"                default\u003dTrue,"},{"line_number":202,"context_line":"                help\u003d_(\u0027This option controls the behavior of the OVN routers \u0027"},{"line_number":203,"context_line":"                       \u0027when handling IPv4 ARP requests or IPv6 ND-NS \u0027"},{"line_number":204,"context_line":"                       \u0027packets. This option is a passthru to set the \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"f29cea62_ba5c5075","line":201,"in_reply_to":"72b2a3dc_46fc3f65","updated":"2021-10-13 08:09:52.000000000","message":"That\u0027s a good conversation to have before we commit this patch. My only experience with these options came from our scale and performance team (which they turned it to False and it helped scaling OVN). In this patch I was being conservative and just had the same default as in core OVN.\n\nBut that said, I do not think that this will be bad on small deployments and we have numbers for large deployments as you said. I think it should be safe to have it as False by default.\n\nThanks for pointing it out!","commit_id":"8c3d5da02a391b6d27cb8009a6d2545ef36091df"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"276f21d6e58b79f36ceefd4c705eb8604cb8bc68","unresolved":true,"context_lines":[{"line_number":198,"context_line":"                       \u0027ovs-appctl -t ovs-vswitchd dpif/show-dp-features \u0027"},{"line_number":199,"context_line":"                       \u0027br-int | grep \"Check pkt length action\".\u0027)),"},{"line_number":200,"context_line":"    cfg.BoolOpt(\u0027router_always_learn_from_arp_request\u0027,"},{"line_number":201,"context_line":"                default\u003dTrue,"},{"line_number":202,"context_line":"                help\u003d_(\u0027This option controls the behavior of the OVN routers \u0027"},{"line_number":203,"context_line":"                       \u0027when handling IPv4 ARP requests or IPv6 ND-NS \u0027"},{"line_number":204,"context_line":"                       \u0027packets. This option is a passthru to set the \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"da9087fa_3795e4b2","line":201,"in_reply_to":"9d5b8f85_8429cfce","updated":"2021-10-14 09:08:12.000000000","message":"Sorry, I mean that we should choose the following values by default.\n\nsetting dynamic_neigh_routers to true \nalways_learn_from_arp_request to false\n\nI\u0027d argue if we even want to make them configurable as I don\u0027t see any scenario where it would help to have different values. WDYT?","commit_id":"8c3d5da02a391b6d27cb8009a6d2545ef36091df"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"02171e4374b6416231e2a19243d9b7326dc0af2f","unresolved":true,"context_lines":[{"line_number":198,"context_line":"                       \u0027ovs-appctl -t ovs-vswitchd dpif/show-dp-features \u0027"},{"line_number":199,"context_line":"                       \u0027br-int | grep \"Check pkt length action\".\u0027)),"},{"line_number":200,"context_line":"    cfg.BoolOpt(\u0027router_always_learn_from_arp_request\u0027,"},{"line_number":201,"context_line":"                default\u003dTrue,"},{"line_number":202,"context_line":"                help\u003d_(\u0027This option controls the behavior of the OVN routers \u0027"},{"line_number":203,"context_line":"                       \u0027when handling IPv4 ARP requests or IPv6 ND-NS \u0027"},{"line_number":204,"context_line":"                       \u0027packets. This option is a passthru to set the \u0027"}],"source_content_type":"text/x-python","patch_set":7,"id":"9d5b8f85_8429cfce","line":201,"in_reply_to":"f29cea62_ba5c5075","updated":"2021-10-14 08:33:22.000000000","message":"Agreed with False being the defaults","commit_id":"8c3d5da02a391b6d27cb8009a6d2545ef36091df"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py":[{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"a3f4b429e069e2e46c5d009afd3d39b7306dd3c4","unresolved":true,"context_lines":[{"line_number":711,"context_line":"    # A static spacing value is used here, but this method will only run"},{"line_number":712,"context_line":"    # once per lock due to the use of periodics.NeverAgain()."},{"line_number":713,"context_line":"    @periodics.periodic(spacing\u003d600, run_immediately\u003dTrue)"},{"line_number":714,"context_line":"    def check_router_always_learn_from_arp_request(self):"},{"line_number":715,"context_line":"        if not self.has_lock:"},{"line_number":716,"context_line":"            return"},{"line_number":717,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"6e8b60c8_d5bb7df0","line":714,"range":{"start_line":714,"start_character":8,"end_line":714,"end_character":50},"updated":"2021-10-12 15:31:58.000000000","message":"damn should change this name","commit_id":"e4c5a41011fe161df25e7d636b54e7f623a310ca"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"9ca761cabd7177dfe25bd81f76ec396c8cdbdf44","unresolved":true,"context_lines":[{"line_number":726,"context_line":"                continue"},{"line_number":727,"context_line":""},{"line_number":728,"context_line":"            opts \u003d copy.deepcopy(router.options)"},{"line_number":729,"context_line":"            opts.update({\u0027always_learn_from_arp_request\u0027: curr_learn_arp})"},{"line_number":730,"context_line":"            opts.update({\u0027dynamic_neigh_routers\u0027: curr_dynamic_neigh})"},{"line_number":731,"context_line":"            cmds.append(self._nb_idl.update_lrouter(router.name, options\u003dopts))"},{"line_number":732,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"b747f634_22a0d647","line":729,"updated":"2021-10-12 16:26:57.000000000","message":"nit: you can do a single update() call with a 2-size dict.","commit_id":"8c3d5da02a391b6d27cb8009a6d2545ef36091df"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"9f40e8d2d623fb1a058d4ccdb9b5c8d7e5b8d89e","unresolved":true,"context_lines":[{"line_number":726,"context_line":"                continue"},{"line_number":727,"context_line":""},{"line_number":728,"context_line":"            opts \u003d copy.deepcopy(router.options)"},{"line_number":729,"context_line":"            opts.update({\u0027always_learn_from_arp_request\u0027: curr_learn_arp})"},{"line_number":730,"context_line":"            opts.update({\u0027dynamic_neigh_routers\u0027: curr_dynamic_neigh})"},{"line_number":731,"context_line":"            cmds.append(self._nb_idl.update_lrouter(router.name, options\u003dopts))"},{"line_number":732,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"981cc1f9_a037ecfc","line":729,"in_reply_to":"b747f634_22a0d647","updated":"2021-10-13 08:09:52.000000000","message":"ops good point!","commit_id":"8c3d5da02a391b6d27cb8009a6d2545ef36091df"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e7edc3b0a92d61e203d7cef91d52c18baf966812","unresolved":true,"context_lines":[{"line_number":718,"context_line":""},{"line_number":719,"context_line":"        cmds \u003d []"},{"line_number":720,"context_line":"        for router in self._nb_idl.lr_list().execute(check_error\u003dTrue):"},{"line_number":721,"context_line":"            if (router.options.get(\u0027always_learn_from_arp_request\u0027) and"},{"line_number":722,"context_line":"                    router.options.get(\u0027dynamic_neigh_routers\u0027)):"},{"line_number":723,"context_line":"                continue"},{"line_number":724,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"fcc25130_583c6b8e","line":721,"updated":"2021-10-14 14:01:52.000000000","message":"to confirm: you check that both keys are set to ANY value, right? Not that they are set to \"correct\" values. This is not immediately clear from the code that this is intentional. Maybe add a comment then?","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"ab2d36e461e906e5a4875f405aba7346a085f038","unresolved":true,"context_lines":[{"line_number":718,"context_line":""},{"line_number":719,"context_line":"        cmds \u003d []"},{"line_number":720,"context_line":"        for router in self._nb_idl.lr_list().execute(check_error\u003dTrue):"},{"line_number":721,"context_line":"            if (router.options.get(\u0027always_learn_from_arp_request\u0027) and"},{"line_number":722,"context_line":"                    router.options.get(\u0027dynamic_neigh_routers\u0027)):"},{"line_number":723,"context_line":"                continue"},{"line_number":724,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"f0317558_4c42e4c9","line":721,"in_reply_to":"6bf7be06_ab1321d9","updated":"2021-10-14 15:34:28.000000000","message":"Based on your reply I assume that you DON\u0027T want admins to set the values and this check is to inspect routers that were created before this patch applied (I agree that admin should not touch it off-band, but wasn\u0027t sure if it\u0027s not a common thing in the driver). Then this check is fine. (Though yes, checking a single value would work too.)\n\nIf you need just one value, it would be slightly more optimal to create a custom ovsdb query filtering just the routers affected. This is just a nice to have since you are saying this method will be removed in the future.","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"766218897d5ca7156da1253e1f5ea6bc7d0a86ba","unresolved":true,"context_lines":[{"line_number":718,"context_line":""},{"line_number":719,"context_line":"        cmds \u003d []"},{"line_number":720,"context_line":"        for router in self._nb_idl.lr_list().execute(check_error\u003dTrue):"},{"line_number":721,"context_line":"            if (router.options.get(\u0027always_learn_from_arp_request\u0027) and"},{"line_number":722,"context_line":"                    router.options.get(\u0027dynamic_neigh_routers\u0027)):"},{"line_number":723,"context_line":"                continue"},{"line_number":724,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"4b943a0f_0f41573d","line":721,"in_reply_to":"f0317558_4c42e4c9","updated":"2021-10-14 16:10:11.000000000","message":"Right, yeah I don\u0027t think we should account for admins setting such values in the ND DB. If we want these to be configurable we should have it as config options like we had before this patch-set","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"f5a68e54b039600d8b125b5c2ece003763cf1873","unresolved":true,"context_lines":[{"line_number":718,"context_line":""},{"line_number":719,"context_line":"        cmds \u003d []"},{"line_number":720,"context_line":"        for router in self._nb_idl.lr_list().execute(check_error\u003dTrue):"},{"line_number":721,"context_line":"            if (router.options.get(\u0027always_learn_from_arp_request\u0027) and"},{"line_number":722,"context_line":"                    router.options.get(\u0027dynamic_neigh_routers\u0027)):"},{"line_number":723,"context_line":"                continue"},{"line_number":724,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"6bf7be06_ab1321d9","line":721,"in_reply_to":"fcc25130_583c6b8e","updated":"2021-10-14 15:26:02.000000000","message":"Right, they idea was just to check whether the options were set or not. I assume that ML2/OVN \"owns\" the database, so the values are set by the create_router() method in ovn_client.py. If the values aren\u0027t set means that this router was create before this patch was in place.\n\nPerhaps I don\u0027t even need to check both keys, since they are included in the same transaction in create_router() if one is set the other one should be set as well.","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"e7edc3b0a92d61e203d7cef91d52c18baf966812","unresolved":true,"context_lines":[{"line_number":723,"context_line":"                continue"},{"line_number":724,"context_line":""},{"line_number":725,"context_line":"            opts \u003d copy.deepcopy(router.options)"},{"line_number":726,"context_line":"            opts.update({\u0027always_learn_from_arp_request\u0027: \u0027false\u0027,"},{"line_number":727,"context_line":"                         \u0027dynamic_neigh_routers\u0027: \u0027true\u0027})"},{"line_number":728,"context_line":"            cmds.append(self._nb_idl.update_lrouter(router.name, options\u003dopts))"},{"line_number":729,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"4442d3ce_c7f16546","line":726,"updated":"2021-10-14 14:01:52.000000000","message":"so if one key was set by an admin off-CMS but not the other, then you override both?.. Still not sure about the intention of yours here. If you want to allow admin to update the keys off-CMS, then why would you override values that were set this way?","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"},{"author":{"_account_id":9656,"name":"Ihar Hrachyshka","email":"ihrachys@redhat.com","username":"ihrachys","status":"Red Hat Networking Systems Engineer"},"change_message_id":"ab2d36e461e906e5a4875f405aba7346a085f038","unresolved":true,"context_lines":[{"line_number":723,"context_line":"                continue"},{"line_number":724,"context_line":""},{"line_number":725,"context_line":"            opts \u003d copy.deepcopy(router.options)"},{"line_number":726,"context_line":"            opts.update({\u0027always_learn_from_arp_request\u0027: \u0027false\u0027,"},{"line_number":727,"context_line":"                         \u0027dynamic_neigh_routers\u0027: \u0027true\u0027})"},{"line_number":728,"context_line":"            cmds.append(self._nb_idl.update_lrouter(router.name, options\u003dopts))"},{"line_number":729,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"c76ad742_ae61d0c3","line":726,"in_reply_to":"30da666d_3ba5031c","updated":"2021-10-14 15:34:28.000000000","message":"I misunderstood the intent here. I understand it now. No, if we don\u0027t allow admins to touch NBDB off-CMS in other places, we shouldn\u0027t consider it here either.","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"f5a68e54b039600d8b125b5c2ece003763cf1873","unresolved":true,"context_lines":[{"line_number":723,"context_line":"                continue"},{"line_number":724,"context_line":""},{"line_number":725,"context_line":"            opts \u003d copy.deepcopy(router.options)"},{"line_number":726,"context_line":"            opts.update({\u0027always_learn_from_arp_request\u0027: \u0027false\u0027,"},{"line_number":727,"context_line":"                         \u0027dynamic_neigh_routers\u0027: \u0027true\u0027})"},{"line_number":728,"context_line":"            cmds.append(self._nb_idl.update_lrouter(router.name, options\u003dopts))"},{"line_number":729,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"30da666d_3ba5031c","line":726,"in_reply_to":"4442d3ce_c7f16546","updated":"2021-10-14 15:26:02.000000000","message":"I usually assume that we won\u0027t have any admin off-CMS changing the OVN Northbound database and ML2/OVN \"owns\" it.\n\nBut in this case, if the admin changed one value and not the other I wouldn\u0027t override any cause in the check on L721 we ignore the values.\n\nDo you think we should account for admin changing thins in Northbound database ? And if so, do we override his values or keep it as it ?\n\nThing is, this method should be removed in the future. This is only used to add these two new options to pre-existing routers created before this patch. So in the future, every router will have these options as they are set by create_router().","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"766218897d5ca7156da1253e1f5ea6bc7d0a86ba","unresolved":true,"context_lines":[{"line_number":723,"context_line":"                continue"},{"line_number":724,"context_line":""},{"line_number":725,"context_line":"            opts \u003d copy.deepcopy(router.options)"},{"line_number":726,"context_line":"            opts.update({\u0027always_learn_from_arp_request\u0027: \u0027false\u0027,"},{"line_number":727,"context_line":"                         \u0027dynamic_neigh_routers\u0027: \u0027true\u0027})"},{"line_number":728,"context_line":"            cmds.append(self._nb_idl.update_lrouter(router.name, options\u003dopts))"},{"line_number":729,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"84035592_15fbf0d4","line":726,"in_reply_to":"c76ad742_ae61d0c3","updated":"2021-10-14 16:10:11.000000000","message":"++ thanks for the review!","commit_id":"a278c5ba789c014ec777a75fc9538179d6707202"}],"neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_maintenance.py":[{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"a3f4b429e069e2e46c5d009afd3d39b7306dd3c4","unresolved":true,"context_lines":[{"line_number":488,"context_line":""},{"line_number":489,"context_line":"        nb_idl.lsp_set_options.assert_has_calls(expected_calls)"},{"line_number":490,"context_line":""},{"line_number":491,"context_line":"    def test_check_router_always_learn_from_arp_request(self):"},{"line_number":492,"context_line":"        # Set the config to False just to vary from the default value"},{"line_number":493,"context_line":"        self.config(group\u003d\u0027ovn\u0027, router_always_learn_from_arp_request\u003dFalse)"},{"line_number":494,"context_line":"        self.config(group\u003d\u0027ovn\u0027, dynamic_neigh_routers\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":5,"id":"c5a5d076_d1870f55","line":491,"range":{"start_line":491,"start_character":13,"end_line":491,"end_character":55},"updated":"2021-10-12 15:31:58.000000000","message":"ditto","commit_id":"e4c5a41011fe161df25e7d636b54e7f623a310ca"}]}