)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"185f23b42dfc623df27043e708af802db632d619","unresolved":false,"context_lines":[{"line_number":22,"context_line":"  to-lport 1002 (outport \u003d\u003d @\u003cPG_ID\u003e \u0026\u0026 ip4 \u0026\u0026 ip4.src \u003d\u003d $\u003cPG_ID\u003e_ip4) allow"},{"line_number":23,"context_line":"  to-lport 1002 (outport \u003d\u003d @\u003cPG_ID\u003e \u0026\u0026 ip6 \u0026\u0026 ip6.src \u003d\u003d $\u003cPG_ID\u003e_ip6) allow"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"A caveat here is that Openstack operators may chosse remove these rules. As VMs"},{"line_number":26,"context_line":"may use IPv4 DHCP to boot, the removal of these security group rules means that"},{"line_number":27,"context_line":"connectivity to DHCP responder (aka ovn-controller) needs to be ensured via an"},{"line_number":28,"context_line":"explicit rule that is not removable by Openstack operators. That is the purpose"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"ff570b3c_097c2770","line":25,"range":{"start_line":25,"start_character":46,"end_line":25,"end_character":52},"updated":"2020-06-04 07:51:02.000000000","message":"choose","commit_id":"af8e81f44c9210f230e509fc8297fb238717987a"}],"doc/source/contributor/internals/ovn/acl_optimizations.rst":[{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"185f23b42dfc623df27043e708af802db632d619","unresolved":false,"context_lines":[{"line_number":192,"context_line":""},{"line_number":193,"context_line":"We don\u0027t want to add this kind of rules to a Port Group since they are"},{"line_number":194,"context_line":"specific to a subnet and not to a Security Group. To overcome this, we will"},{"line_number":195,"context_line":"create a Port Group every time a subnet gets created (pg-sub-\u003csubnet_uuid\u003e)"},{"line_number":196,"context_line":"and add the ACL to it."},{"line_number":197,"context_line":""},{"line_number":198,"context_line":"When the port is created/updated, we\u0027ll add it to those subnet Port Groups"}],"source_content_type":"text/x-rst","patch_set":4,"id":"ff570b3c_a9ed3b26","line":195,"range":{"start_line":195,"start_character":33,"end_line":195,"end_character":52},"updated":"2020-06-04 07:51:02.000000000","message":"I would say we need this PG and rules while we have subnet AND the enable-dhcp set on it.","commit_id":"af8e81f44c9210f230e509fc8297fb238717987a"}],"neutron/common/ovn/acl.py":[{"author":{"_account_id":24791,"name":"Maciej Jozefczyk","email":"jeicam.pl@gmail.com","username":"maciej.jozefczyk"},"change_message_id":"185f23b42dfc623df27043e708af802db632d619","unresolved":false,"context_lines":[{"line_number":158,"context_line":"    # Allow DHCP requests for OVN native DHCP service, while responses are"},{"line_number":159,"context_line":"    # allowed in ovn-northd."},{"line_number":160,"context_line":"    # Allow both DHCP requests and responses to pass for other DHCP services."},{"line_number":161,"context_line":"    # We do this even if DHCP isn\u0027t enabled for the subnet"},{"line_number":162,"context_line":"    acl_list \u003d []"},{"line_number":163,"context_line":"    if subnet[\u0027ip_version\u0027] \u003d\u003d const.IP_VERSION_4:"},{"line_number":164,"context_line":"        if not ovn_dhcp:"}],"source_content_type":"text/x-python","patch_set":4,"id":"ff570b3c_49c73f97","line":161,"range":{"start_line":161,"start_character":4,"end_line":161,"end_character":58},"updated":"2020-06-04 07:51:02.000000000","message":"Question for other reviewers: Hmm, is that the same for ml2/ovs that we install \"UDP 67-68 ALLOW\" even tho the DHCP is disabled on subnet?","commit_id":"af8e81f44c9210f230e509fc8297fb238717987a"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"091e7306e3a8712eb4595238fe349fd807c6db2c","unresolved":false,"context_lines":[{"line_number":190,"context_line":"            acl \u003d {\"port_group\": pg_name,"},{"line_number":191,"context_line":"                   \"priority\": ovn_const.ACL_PRIORITY_ALLOW,"},{"line_number":192,"context_line":"                   \"action\": ovn_const.ACL_ACTION_ALLOW, \"log\": False,"},{"line_number":193,"context_line":"                   \"name\": [], \"severity\": [], \"direction\": \u0027to-lport\u0027,"},{"line_number":194,"context_line":"                   \"match\": (\u0027outport \u003d\u003d @%s \u0026\u0026 ip6 \u0026\u0026 ip6.src \u003d\u003d %s \u0026\u0026 \u0027"},{"line_number":195,"context_line":"                             \u0027udp \u0026\u0026 udp.src \u003d\u003d 547 \u0026\u0026 udp.dst \u003d\u003d 546\u0027) % ("},{"line_number":196,"context_line":"                            pg_name, subnet[\u0027cidr\u0027])}"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff570b3c_119c2c68","line":193,"updated":"2020-06-05 20:26:32.000000000","message":"nit: can you put each of these on their own line? Just makes it easier to look quickly and verify arguments.","commit_id":"19cd35947c3f01ba6c32e2318951c0383cd25993"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"938252ca8c35ccdc1c8ea5f04e2bff3d391747bf","unresolved":false,"context_lines":[{"line_number":190,"context_line":"            acl \u003d {\"port_group\": pg_name,"},{"line_number":191,"context_line":"                   \"priority\": ovn_const.ACL_PRIORITY_ALLOW,"},{"line_number":192,"context_line":"                   \"action\": ovn_const.ACL_ACTION_ALLOW, \"log\": False,"},{"line_number":193,"context_line":"                   \"name\": [], \"severity\": [], \"direction\": \u0027to-lport\u0027,"},{"line_number":194,"context_line":"                   \"match\": (\u0027outport \u003d\u003d @%s \u0026\u0026 ip6 \u0026\u0026 ip6.src \u003d\u003d %s \u0026\u0026 \u0027"},{"line_number":195,"context_line":"                             \u0027udp \u0026\u0026 udp.src \u003d\u003d 547 \u0026\u0026 udp.dst \u003d\u003d 546\u0027) % ("},{"line_number":196,"context_line":"                            pg_name, subnet[\u0027cidr\u0027])}"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff570b3c_fc6a2c7e","line":193,"in_reply_to":"ff570b3c_119c2c68","updated":"2020-06-06 11:45:57.000000000","message":"Done","commit_id":"19cd35947c3f01ba6c32e2318951c0383cd25993"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"091e7306e3a8712eb4595238fe349fd807c6db2c","unresolved":false,"context_lines":[{"line_number":192,"context_line":"                   \"action\": ovn_const.ACL_ACTION_ALLOW, \"log\": False,"},{"line_number":193,"context_line":"                   \"name\": [], \"severity\": [], \"direction\": \u0027to-lport\u0027,"},{"line_number":194,"context_line":"                   \"match\": (\u0027outport \u003d\u003d @%s \u0026\u0026 ip6 \u0026\u0026 ip6.src \u003d\u003d %s \u0026\u0026 \u0027"},{"line_number":195,"context_line":"                             \u0027udp \u0026\u0026 udp.src \u003d\u003d 547 \u0026\u0026 udp.dst \u003d\u003d 546\u0027) % ("},{"line_number":196,"context_line":"                            pg_name, subnet[\u0027cidr\u0027])}"},{"line_number":197,"context_line":"            acl_list.append(acl)"},{"line_number":198,"context_line":"        acl \u003d {\"port_group\": pg_name,"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff570b3c_318e902d","line":195,"range":{"start_line":195,"start_character":48,"end_line":195,"end_character":51},"updated":"2020-06-05 20:26:32.000000000","message":"Is the order here correct? I think src port should be 546, dst 547 for \u0027to-lport\u0027, and the opposite below.  At least that\u0027s how you did the IPv4 ones above.","commit_id":"19cd35947c3f01ba6c32e2318951c0383cd25993"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"938252ca8c35ccdc1c8ea5f04e2bff3d391747bf","unresolved":false,"context_lines":[{"line_number":192,"context_line":"                   \"action\": ovn_const.ACL_ACTION_ALLOW, \"log\": False,"},{"line_number":193,"context_line":"                   \"name\": [], \"severity\": [], \"direction\": \u0027to-lport\u0027,"},{"line_number":194,"context_line":"                   \"match\": (\u0027outport \u003d\u003d @%s \u0026\u0026 ip6 \u0026\u0026 ip6.src \u003d\u003d %s \u0026\u0026 \u0027"},{"line_number":195,"context_line":"                             \u0027udp \u0026\u0026 udp.src \u003d\u003d 547 \u0026\u0026 udp.dst \u003d\u003d 546\u0027) % ("},{"line_number":196,"context_line":"                            pg_name, subnet[\u0027cidr\u0027])}"},{"line_number":197,"context_line":"            acl_list.append(acl)"},{"line_number":198,"context_line":"        acl \u003d {\"port_group\": pg_name,"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff570b3c_1c09e0db","line":195,"range":{"start_line":195,"start_character":48,"end_line":195,"end_character":51},"in_reply_to":"ff570b3c_318e902d","updated":"2020-06-06 11:45:57.000000000","message":"Interesting comment ;) I used firewall.py for ovs as reference and now wonder if they are reversed there as well.\n\nhttps://github.com/openstack/neutron/blob/64b92687a56b4176e65ef7ab1520f894e82d45e5/neutron/agent/linux/openvswitch_firewall/firewall.py#L1229\n\nI also double checked rfcs from https://en.wikipedia.org/wiki/DHCPv6#Port_numbers\n\nI am pretty sure that srv uses 547 and client uses 546. The\ncode is adds rules for both directions for cases when ovn is not running as the dhcp responder.","commit_id":"19cd35947c3f01ba6c32e2318951c0383cd25993"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"30f85265896d0b98a8edf32cbadb2ac936ccd0c2","unresolved":false,"context_lines":[{"line_number":161,"context_line":"    acl_list \u003d []"},{"line_number":162,"context_line":"    if subnet[\u0027ip_version\u0027] \u003d\u003d const.IP_VERSION_4:"},{"line_number":163,"context_line":"        if not ovn_dhcp:"},{"line_number":164,"context_line":"            acl \u003d {\"port_group\": pg_name,"},{"line_number":165,"context_line":"                   \"priority\": ovn_const.ACL_PRIORITY_ALLOW,"},{"line_number":166,"context_line":"                   \"action\": ovn_const.ACL_ACTION_ALLOW,"},{"line_number":167,"context_line":"                   \"log\": False,"},{"line_number":168,"context_line":"                   \"name\": [],"},{"line_number":169,"context_line":"                   \"severity\": [],"},{"line_number":170,"context_line":"                   \"direction\": \u0027to-lport\u0027,"},{"line_number":171,"context_line":"                   \"match\": (\u0027outport \u003d\u003d @%s \u0026\u0026 ip4 \u0026\u0026 ip4.src \u003d\u003d %s \u0026\u0026 \u0027"},{"line_number":172,"context_line":"                             \u0027udp \u0026\u0026 udp.src \u003d\u003d 67 \u0026\u0026 udp.dst \u003d\u003d 68\u0027"},{"line_number":173,"context_line":"                             ) % (pg_name, subnet[\u0027cidr\u0027])}"},{"line_number":174,"context_line":"            acl_list.append(acl)"},{"line_number":175,"context_line":"        acl \u003d {\"port_group\": pg_name,"},{"line_number":176,"context_line":"               \"priority\": ovn_const.ACL_PRIORITY_ALLOW,"},{"line_number":177,"context_line":"               \"action\": ovn_const.ACL_ACTION_ALLOW,"},{"line_number":178,"context_line":"               \"log\": False,"},{"line_number":179,"context_line":"               \"name\": [],"},{"line_number":180,"context_line":"               \"severity\": [],"},{"line_number":181,"context_line":"               \"direction\": \u0027from-lport\u0027,"},{"line_number":182,"context_line":"               \"match\": (\u0027inport \u003d\u003d @%s \u0026\u0026 ip4 \u0026\u0026 \u0027"},{"line_number":183,"context_line":"                         \u0027ip4.dst \u003d\u003d {255.255.255.255, %s} \u0026\u0026 \u0027"},{"line_number":184,"context_line":"                         \u0027udp \u0026\u0026 udp.src \u003d\u003d 68 \u0026\u0026 udp.dst \u003d\u003d 67\u0027"},{"line_number":185,"context_line":"                         ) % (pg_name, subnet[\u0027cidr\u0027])}"},{"line_number":186,"context_line":"        acl_list.append(acl)"},{"line_number":187,"context_line":"    elif subnet[\u0027ip_version\u0027] \u003d\u003d const.IP_VERSION_6:"},{"line_number":188,"context_line":"        if not ovn_dhcp:"},{"line_number":189,"context_line":"            acl \u003d {\"port_group\": pg_name,"},{"line_number":190,"context_line":"                   \"priority\": ovn_const.ACL_PRIORITY_ALLOW,"},{"line_number":191,"context_line":"                   \"action\": ovn_const.ACL_ACTION_ALLOW,"},{"line_number":192,"context_line":"                   \"log\": False,"},{"line_number":193,"context_line":"                   \"name\": [],"},{"line_number":194,"context_line":"                   \"severity\": [],"},{"line_number":195,"context_line":"                   \"direction\": \u0027to-lport\u0027,"},{"line_number":196,"context_line":"                   \"match\": (\u0027outport \u003d\u003d @%s \u0026\u0026 ip6 \u0026\u0026 ip6.src \u003d\u003d %s \u0026\u0026 \u0027"},{"line_number":197,"context_line":"                             \u0027udp \u0026\u0026 udp.src \u003d\u003d 547 \u0026\u0026 udp.dst \u003d\u003d 546\u0027) % ("},{"line_number":198,"context_line":"                            pg_name, subnet[\u0027cidr\u0027])}"},{"line_number":199,"context_line":"            acl_list.append(acl)"},{"line_number":200,"context_line":"        acl \u003d {\"port_group\": pg_name,"},{"line_number":201,"context_line":"               \"priority\": ovn_const.ACL_PRIORITY_ALLOW,"},{"line_number":202,"context_line":"               \"action\": ovn_const.ACL_ACTION_ALLOW,"},{"line_number":203,"context_line":"               \"log\": False,"},{"line_number":204,"context_line":"               \"name\": [],"},{"line_number":205,"context_line":"               \"severity\": [],"},{"line_number":206,"context_line":"               \"direction\": \u0027from-lport\u0027,"},{"line_number":207,"context_line":"               \"match\": (\u0027inport \u003d\u003d @%s \u0026\u0026 ip6 \u0026\u0026 \u0027"},{"line_number":208,"context_line":"                         \u0027ip6.dst \u003d\u003d {ff02::1:2, %s} \u0026\u0026 \u0027"},{"line_number":209,"context_line":"                         \u0027udp \u0026\u0026 udp.src \u003d\u003d 546 \u0026\u0026 udp.dst \u003d\u003d 547\u0027) % ("},{"line_number":210,"context_line":"                        pg_name, subnet[\u0027cidr\u0027])}"},{"line_number":211,"context_line":"        acl_list.append(acl)"},{"line_number":212,"context_line":"    return acl_list"},{"line_number":213,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"ff570b3c_56bae509","line":210,"range":{"start_line":164,"start_character":0,"end_line":210,"end_character":49},"updated":"2020-06-09 21:35:07.000000000","message":"nit: a lot of repetition here with most of the fields being identical. Doing the non-lamba equivalent of:\n\n acl \u003d lambda direction, match: {\u0027port_group\u0027: pg_name, \u0027priority\u0027: 1, \u0027action\u0027: \u0027allow\u0027, \u0027log\u0027: False, \u0027name\u0027: [], \u0027severity\u0027: [], \u0027direction\u0027: direction, \u0027match\u0027: match}\n acl(\u0027to-lport\u0027, \u0027outport \u003d\u003d ...\u0027)\n\nmight make for a little less reading.","commit_id":"4a017675b1645509cf5c437ef3fc6f2914b6030d"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"f327502a7297bb641aff749ac9ee240197471cb6","unresolved":false,"context_lines":[{"line_number":161,"context_line":"    acl_list \u003d []"},{"line_number":162,"context_line":"    if subnet[\u0027ip_version\u0027] \u003d\u003d const.IP_VERSION_4:"},{"line_number":163,"context_line":"        if not ovn_dhcp:"},{"line_number":164,"context_line":"            acl \u003d {\"port_group\": pg_name,"},{"line_number":165,"context_line":"                   \"priority\": ovn_const.ACL_PRIORITY_ALLOW,"},{"line_number":166,"context_line":"                   \"action\": ovn_const.ACL_ACTION_ALLOW,"},{"line_number":167,"context_line":"                   \"log\": False,"},{"line_number":168,"context_line":"                   \"name\": [],"},{"line_number":169,"context_line":"                   \"severity\": [],"},{"line_number":170,"context_line":"                   \"direction\": \u0027to-lport\u0027,"},{"line_number":171,"context_line":"                   \"match\": (\u0027outport \u003d\u003d @%s \u0026\u0026 ip4 \u0026\u0026 ip4.src \u003d\u003d %s \u0026\u0026 \u0027"},{"line_number":172,"context_line":"                             \u0027udp \u0026\u0026 udp.src \u003d\u003d 67 \u0026\u0026 udp.dst \u003d\u003d 68\u0027"},{"line_number":173,"context_line":"                             ) % (pg_name, subnet[\u0027cidr\u0027])}"},{"line_number":174,"context_line":"            acl_list.append(acl)"},{"line_number":175,"context_line":"        acl \u003d {\"port_group\": pg_name,"},{"line_number":176,"context_line":"               \"priority\": ovn_const.ACL_PRIORITY_ALLOW,"},{"line_number":177,"context_line":"               \"action\": ovn_const.ACL_ACTION_ALLOW,"},{"line_number":178,"context_line":"               \"log\": False,"},{"line_number":179,"context_line":"               \"name\": [],"},{"line_number":180,"context_line":"               \"severity\": [],"},{"line_number":181,"context_line":"               \"direction\": \u0027from-lport\u0027,"},{"line_number":182,"context_line":"               \"match\": (\u0027inport \u003d\u003d @%s \u0026\u0026 ip4 \u0026\u0026 \u0027"},{"line_number":183,"context_line":"                         \u0027ip4.dst \u003d\u003d {255.255.255.255, %s} \u0026\u0026 \u0027"},{"line_number":184,"context_line":"                         \u0027udp \u0026\u0026 udp.src \u003d\u003d 68 \u0026\u0026 udp.dst \u003d\u003d 67\u0027"},{"line_number":185,"context_line":"                         ) % (pg_name, subnet[\u0027cidr\u0027])}"},{"line_number":186,"context_line":"        acl_list.append(acl)"},{"line_number":187,"context_line":"    elif subnet[\u0027ip_version\u0027] \u003d\u003d const.IP_VERSION_6:"},{"line_number":188,"context_line":"        if not ovn_dhcp:"},{"line_number":189,"context_line":"            acl \u003d {\"port_group\": pg_name,"},{"line_number":190,"context_line":"                   \"priority\": ovn_const.ACL_PRIORITY_ALLOW,"},{"line_number":191,"context_line":"                   \"action\": ovn_const.ACL_ACTION_ALLOW,"},{"line_number":192,"context_line":"                   \"log\": False,"},{"line_number":193,"context_line":"                   \"name\": [],"},{"line_number":194,"context_line":"                   \"severity\": [],"},{"line_number":195,"context_line":"                   \"direction\": \u0027to-lport\u0027,"},{"line_number":196,"context_line":"                   \"match\": (\u0027outport \u003d\u003d @%s \u0026\u0026 ip6 \u0026\u0026 ip6.src \u003d\u003d %s \u0026\u0026 \u0027"},{"line_number":197,"context_line":"                             \u0027udp \u0026\u0026 udp.src \u003d\u003d 547 \u0026\u0026 udp.dst \u003d\u003d 546\u0027) % ("},{"line_number":198,"context_line":"                            pg_name, subnet[\u0027cidr\u0027])}"},{"line_number":199,"context_line":"            acl_list.append(acl)"},{"line_number":200,"context_line":"        acl \u003d {\"port_group\": pg_name,"},{"line_number":201,"context_line":"               \"priority\": ovn_const.ACL_PRIORITY_ALLOW,"},{"line_number":202,"context_line":"               \"action\": ovn_const.ACL_ACTION_ALLOW,"},{"line_number":203,"context_line":"               \"log\": False,"},{"line_number":204,"context_line":"               \"name\": [],"},{"line_number":205,"context_line":"               \"severity\": [],"},{"line_number":206,"context_line":"               \"direction\": \u0027from-lport\u0027,"},{"line_number":207,"context_line":"               \"match\": (\u0027inport \u003d\u003d @%s \u0026\u0026 ip6 \u0026\u0026 \u0027"},{"line_number":208,"context_line":"                         \u0027ip6.dst \u003d\u003d {ff02::1:2, %s} \u0026\u0026 \u0027"},{"line_number":209,"context_line":"                         \u0027udp \u0026\u0026 udp.src \u003d\u003d 546 \u0026\u0026 udp.dst \u003d\u003d 547\u0027) % ("},{"line_number":210,"context_line":"                        pg_name, subnet[\u0027cidr\u0027])}"},{"line_number":211,"context_line":"        acl_list.append(acl)"},{"line_number":212,"context_line":"    return acl_list"},{"line_number":213,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"ff570b3c_4a17d152","line":210,"range":{"start_line":164,"start_character":0,"end_line":210,"end_character":49},"in_reply_to":"ff570b3c_56bae509","updated":"2020-06-10 10:33:43.000000000","message":"yeah. I was following the format of the other acls in this file, but agree. Some folks prefer each field on its own separate line to me easier on the eyes.","commit_id":"4a017675b1645509cf5c437ef3fc6f2914b6030d"}],"neutron/common/ovn/utils.py":[{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"279df9104df7ade41fc5a92b85ed4e0dd0052c6e","unresolved":false,"context_lines":[{"line_number":115,"context_line":""},{"line_number":116,"context_line":""},{"line_number":117,"context_line":"def is_ovn_subnet_port_group_name(sg_name):"},{"line_number":118,"context_line":"    return sg_name.startswith(ovn_subnet_port_group_name(\u0027\u0027))"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"def is_subnet_pg_required(subnet):"}],"source_content_type":"text/x-python","patch_set":16,"id":"ff570b3c_a9614614","line":118,"range":{"start_line":118,"start_character":30,"end_line":118,"end_character":60},"updated":"2020-06-11 09:18:25.000000000","message":"nit: Maybe create a constant that can be used for both methods ?\n\ne.g: https://github.com/openstack/neutron/blob/76b2ace0790e3f5df19f7d4425aa26c966b373a6/neutron/common/ovn/constants.py#L60","commit_id":"340019235e8c773d20e3bcf24dce929775241ba3"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"ebd416bc517a9b8be27fbb209823f4bb3f5a22d9","unresolved":false,"context_lines":[{"line_number":115,"context_line":""},{"line_number":116,"context_line":""},{"line_number":117,"context_line":"def is_ovn_subnet_port_group_name(sg_name):"},{"line_number":118,"context_line":"    return sg_name.startswith(ovn_subnet_port_group_name(\u0027\u0027))"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"def is_subnet_pg_required(subnet):"}],"source_content_type":"text/x-python","patch_set":16,"id":"ff570b3c_bf67adea","line":118,"range":{"start_line":118,"start_character":30,"end_line":118,"end_character":60},"in_reply_to":"ff570b3c_a9614614","updated":"2020-06-11 19:23:46.000000000","message":"Good suggestion. Done","commit_id":"340019235e8c773d20e3bcf24dce929775241ba3"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py":[{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"3aaeb99282f2d8a4ffbcc473da37cfc1fa93a396","unresolved":false,"context_lines":[{"line_number":698,"context_line":"        if uuidutils.is_uuid_like(pg_name):"},{"line_number":699,"context_line":"            pg_name \u003d utils.ovn_port_group_name(pg_name)"},{"line_number":700,"context_line":"        try:"},{"line_number":701,"context_line":"            pg_cmd \u003d self.db_find_rows(\u0027Port_Group\u0027, (\u0027name\u0027, \u0027\u003d\u0027, pg_name))"},{"line_number":702,"context_line":"            result \u003d pg_cmd.execute(check_error\u003dTrue)"},{"line_number":703,"context_line":"            return result[0] if result else None"},{"line_number":704,"context_line":"        except KeyError:"},{"line_number":705,"context_line":"            # TODO(dalvarez): This except block is added for backwards compat"},{"line_number":706,"context_line":"            # with old OVN schemas (\u003c\u003d2.9) where Port Groups are not present."}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_79f43188","line":703,"range":{"start_line":701,"start_character":0,"end_line":703,"end_character":48},"updated":"2020-06-11 12:21:16.000000000","message":"If you leverage the name of the subnet PG by name you will have unique names for PG which is much cleaner IMHO.","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"c51932f45240cac9624bd9a84a9134821e0d37d0","unresolved":false,"context_lines":[{"line_number":685,"context_line":"            # removed at some point."},{"line_number":686,"context_line":"            return"},{"line_number":687,"context_line":""},{"line_number":688,"context_line":"    # TODO(flaviof): maybe this should be renamed to get_sg_port_groups?"},{"line_number":689,"context_line":"    def get_port_groups(self):"},{"line_number":690,"context_line":"        port_groups \u003d {}"},{"line_number":691,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":20,"id":"bf51134e_36a81b1e","line":688,"range":{"start_line":688,"start_character":4,"end_line":688,"end_character":72},"updated":"2020-06-16 09:56:12.000000000","message":"I think so :) Also with a oneline docstring explaining :?","commit_id":"888bbae055a48844a0badaf361605fe127a81b22"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"8eff061e3f08961eb87af80c20370b96e16b336b","unresolved":false,"context_lines":[{"line_number":685,"context_line":"            # removed at some point."},{"line_number":686,"context_line":"            return"},{"line_number":687,"context_line":""},{"line_number":688,"context_line":"    # TODO(flaviof): maybe this should be renamed to get_sg_port_groups?"},{"line_number":689,"context_line":"    def get_port_groups(self):"},{"line_number":690,"context_line":"        port_groups \u003d {}"},{"line_number":691,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":20,"id":"bf51134e_4e47f31e","line":688,"range":{"start_line":688,"start_character":4,"end_line":688,"end_character":72},"in_reply_to":"bf51134e_36a81b1e","updated":"2020-06-16 19:32:05.000000000","message":"https://review.opendev.org/#/c/735925/\nbug: https://bugs.launchpad.net/networking-ovn/+bug/1883716","commit_id":"888bbae055a48844a0badaf361605fe127a81b22"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/maintenance.py":[{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"2665a33d8ed7fc6e4d60b7f50e8804f4c338bd84","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            res_map[\u0027ovn_delete\u0027](context, row.resource_uuid)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def _fix_create_update_subnet(self, context, row):"},{"line_number":269,"context_line":"        # Get the lasted version of the port in Neutron DB"},{"line_number":270,"context_line":"        sn_db_obj \u003d self._ovn_client._plugin.get_subnet("},{"line_number":271,"context_line":"            context, row.resource_uuid)"},{"line_number":272,"context_line":"        n_db_obj \u003d self._ovn_client._plugin.get_network("},{"line_number":273,"context_line":"            context, sn_db_obj[\u0027network_id\u0027])"},{"line_number":274,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"ff570b3c_b6c0ef51","line":271,"range":{"start_line":269,"start_character":6,"end_line":271,"end_character":39},"updated":"2020-06-04 15:44:17.000000000","message":"We need to do some magic here as now a subnet in Neutron maps to two things in OVN ... \n\nThis comment is related to the other comment I left in this patch","commit_id":"42ca5dd6d00bf8b34eb63ac760bf68002e58f238"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"938252ca8c35ccdc1c8ea5f04e2bff3d391747bf","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            res_map[\u0027ovn_delete\u0027](context, row.resource_uuid)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def _fix_create_update_subnet(self, context, row):"},{"line_number":269,"context_line":"        # Get the lasted version of the port in Neutron DB"},{"line_number":270,"context_line":"        sn_db_obj \u003d self._ovn_client._plugin.get_subnet("},{"line_number":271,"context_line":"            context, row.resource_uuid)"},{"line_number":272,"context_line":"        n_db_obj \u003d self._ovn_client._plugin.get_network("},{"line_number":273,"context_line":"            context, sn_db_obj[\u0027network_id\u0027])"},{"line_number":274,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"ff570b3c_7c573ccb","line":271,"range":{"start_line":269,"start_character":6,"end_line":271,"end_character":39},"in_reply_to":"ff570b3c_76977746","updated":"2020-06-06 11:45:57.000000000","message":"Right. Subnet pg piggy backs on dhcp_options. That ensures the sync with neutron DB for that subnet.","commit_id":"42ca5dd6d00bf8b34eb63ac760bf68002e58f238"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"938252ca8c35ccdc1c8ea5f04e2bff3d391747bf","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            res_map[\u0027ovn_delete\u0027](context, row.resource_uuid)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def _fix_create_update_subnet(self, context, row):"},{"line_number":269,"context_line":"        # Get the lasted version of the port in Neutron DB"},{"line_number":270,"context_line":"        sn_db_obj \u003d self._ovn_client._plugin.get_subnet("},{"line_number":271,"context_line":"            context, row.resource_uuid)"},{"line_number":272,"context_line":"        n_db_obj \u003d self._ovn_client._plugin.get_network("},{"line_number":273,"context_line":"            context, sn_db_obj[\u0027network_id\u0027])"},{"line_number":274,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"ff570b3c_bcf9d4c8","line":271,"range":{"start_line":269,"start_character":6,"end_line":271,"end_character":39},"in_reply_to":"ff570b3c_b6c0ef51","updated":"2020-06-06 11:45:57.000000000","message":"Done","commit_id":"42ca5dd6d00bf8b34eb63ac760bf68002e58f238"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"2af5afe580bc903f5e4393fd997879d074f9c152","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            res_map[\u0027ovn_delete\u0027](context, row.resource_uuid)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def _fix_create_update_subnet(self, context, row):"},{"line_number":269,"context_line":"        # Get the lasted version of the port in Neutron DB"},{"line_number":270,"context_line":"        sn_db_obj \u003d self._ovn_client._plugin.get_subnet("},{"line_number":271,"context_line":"            context, row.resource_uuid)"},{"line_number":272,"context_line":"        n_db_obj \u003d self._ovn_client._plugin.get_network("},{"line_number":273,"context_line":"            context, sn_db_obj[\u0027network_id\u0027])"},{"line_number":274,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"ff570b3c_02386ed0","line":271,"range":{"start_line":269,"start_character":6,"end_line":271,"end_character":39},"in_reply_to":"ff570b3c_b6c0ef51","updated":"2020-06-04 21:29:35.000000000","message":"Let me dig your comment and join it with this one...\n\n\u003e We will also have to account for the subnet port group in the maintenance task adding the external id of the subnet to the port group.\n\u003e Now every neutron subnet will be associated to a) DHCP_Options row and b) Subnet Port Group.\n\n\nack.\n\n\u003e Also the existing ports belonging to a subnet shall be added to the port group of the subnet they belong to upon start.\n\u003e\n\ndone.","commit_id":"42ca5dd6d00bf8b34eb63ac760bf68002e58f238"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"fc97db1da0d58664e7d4006fec92afbfcd0b82f6","unresolved":false,"context_lines":[{"line_number":266,"context_line":"            res_map[\u0027ovn_delete\u0027](context, row.resource_uuid)"},{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    def _fix_create_update_subnet(self, context, row):"},{"line_number":269,"context_line":"        # Get the lasted version of the port in Neutron DB"},{"line_number":270,"context_line":"        sn_db_obj \u003d self._ovn_client._plugin.get_subnet("},{"line_number":271,"context_line":"            context, row.resource_uuid)"},{"line_number":272,"context_line":"        n_db_obj \u003d self._ovn_client._plugin.get_network("},{"line_number":273,"context_line":"            context, sn_db_obj[\u0027network_id\u0027])"},{"line_number":274,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"ff570b3c_76977746","line":271,"range":{"start_line":269,"start_character":6,"end_line":271,"end_character":39},"in_reply_to":"ff570b3c_b6c0ef51","updated":"2020-06-04 15:47:42.000000000","message":"Thinking twice perhaps it\u0027s not needed as long as we create the subnet pg and the dhcp_options row *in the same OVN transaction*","commit_id":"42ca5dd6d00bf8b34eb63ac760bf68002e58f238"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"2132c42b6e39dd1c19f9dec859837eac28d152cd","unresolved":false,"context_lines":[{"line_number":615,"context_line":"                pg_name \u003d utils.ovn_port_group_name(subnet_id)"},{"line_number":616,"context_line":"                # If subnet Port Group doesn\u0027t exist yet, create it."},{"line_number":617,"context_line":"                if not self._nb_idl.get_port_group(pg_name):"},{"line_number":618,"context_line":"                    LOG.debug(\u0027Adding subnet port group %s\u0027, pg_name)"},{"line_number":619,"context_line":"                    ext_ids \u003d {"},{"line_number":620,"context_line":"                        ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name("},{"line_number":621,"context_line":"                            subnet[\u0027network_id\u0027])}"},{"line_number":622,"context_line":"                    txn.add(self._nb_idl.pg_add(pg_name, acls\u003d[],"},{"line_number":623,"context_line":"                                                external_ids\u003dext_ids))"},{"line_number":624,"context_line":"                    # Add ACLs to this Port Group."},{"line_number":625,"context_line":"                    acls \u003d acl_utils.add_acls_for_subnet_port_group("},{"line_number":626,"context_line":"                        pg_name, subnet)"},{"line_number":627,"context_line":"                    for acl in acls:"},{"line_number":628,"context_line":"                        txn.add(self._nb_idl.pg_acl_add(**acl))"},{"line_number":629,"context_line":"            # Add ports to the relevant subnet Port Groups"},{"line_number":630,"context_line":"            for port in db_ports:"},{"line_number":631,"context_line":"                for ip in port[\u0027fixed_ips\u0027]:"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff570b3c_a909a22b","line":628,"range":{"start_line":618,"start_character":0,"end_line":628,"end_character":63},"updated":"2020-06-05 10:31:59.000000000","message":"Can\u0027t we re-use the code in OVNCLient._create_subnet_port_group() for this ?","commit_id":"3bf5d3672b50448d9731f9dc5c59d58aec60e66e"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"938252ca8c35ccdc1c8ea5f04e2bff3d391747bf","unresolved":false,"context_lines":[{"line_number":615,"context_line":"                pg_name \u003d utils.ovn_port_group_name(subnet_id)"},{"line_number":616,"context_line":"                # If subnet Port Group doesn\u0027t exist yet, create it."},{"line_number":617,"context_line":"                if not self._nb_idl.get_port_group(pg_name):"},{"line_number":618,"context_line":"                    LOG.debug(\u0027Adding subnet port group %s\u0027, pg_name)"},{"line_number":619,"context_line":"                    ext_ids \u003d {"},{"line_number":620,"context_line":"                        ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name("},{"line_number":621,"context_line":"                            subnet[\u0027network_id\u0027])}"},{"line_number":622,"context_line":"                    txn.add(self._nb_idl.pg_add(pg_name, acls\u003d[],"},{"line_number":623,"context_line":"                                                external_ids\u003dext_ids))"},{"line_number":624,"context_line":"                    # Add ACLs to this Port Group."},{"line_number":625,"context_line":"                    acls \u003d acl_utils.add_acls_for_subnet_port_group("},{"line_number":626,"context_line":"                        pg_name, subnet)"},{"line_number":627,"context_line":"                    for acl in acls:"},{"line_number":628,"context_line":"                        txn.add(self._nb_idl.pg_acl_add(**acl))"},{"line_number":629,"context_line":"            # Add ports to the relevant subnet Port Groups"},{"line_number":630,"context_line":"            for port in db_ports:"},{"line_number":631,"context_line":"                for ip in port[\u0027fixed_ips\u0027]:"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff570b3c_1c6440b8","line":628,"range":{"start_line":618,"start_character":0,"end_line":628,"end_character":63},"in_reply_to":"ff570b3c_50bd02d6","updated":"2020-06-06 11:45:57.000000000","message":"Done","commit_id":"3bf5d3672b50448d9731f9dc5c59d58aec60e66e"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"c957904032e2393c7e459ca002f9205f2dbd2248","unresolved":false,"context_lines":[{"line_number":615,"context_line":"                pg_name \u003d utils.ovn_port_group_name(subnet_id)"},{"line_number":616,"context_line":"                # If subnet Port Group doesn\u0027t exist yet, create it."},{"line_number":617,"context_line":"                if not self._nb_idl.get_port_group(pg_name):"},{"line_number":618,"context_line":"                    LOG.debug(\u0027Adding subnet port group %s\u0027, pg_name)"},{"line_number":619,"context_line":"                    ext_ids \u003d {"},{"line_number":620,"context_line":"                        ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name("},{"line_number":621,"context_line":"                            subnet[\u0027network_id\u0027])}"},{"line_number":622,"context_line":"                    txn.add(self._nb_idl.pg_add(pg_name, acls\u003d[],"},{"line_number":623,"context_line":"                                                external_ids\u003dext_ids))"},{"line_number":624,"context_line":"                    # Add ACLs to this Port Group."},{"line_number":625,"context_line":"                    acls \u003d acl_utils.add_acls_for_subnet_port_group("},{"line_number":626,"context_line":"                        pg_name, subnet)"},{"line_number":627,"context_line":"                    for acl in acls:"},{"line_number":628,"context_line":"                        txn.add(self._nb_idl.pg_acl_add(**acl))"},{"line_number":629,"context_line":"            # Add ports to the relevant subnet Port Groups"},{"line_number":630,"context_line":"            for port in db_ports:"},{"line_number":631,"context_line":"                for ip in port[\u0027fixed_ips\u0027]:"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff570b3c_50bd02d6","line":628,"range":{"start_line":618,"start_character":0,"end_line":628,"end_character":63},"in_reply_to":"ff570b3c_a909a22b","updated":"2020-06-05 16:59:14.000000000","message":"yes, we can! Will fix that.","commit_id":"3bf5d3672b50448d9731f9dc5c59d58aec60e66e"},{"author":{"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]},"tag":"autogenerated:zuul:check","change_message_id":"74f28f8d9f86f28422efb498a66309bb30a8f6d9","unresolved":false,"context_lines":[{"line_number":27,"context_line":"from oslo_utils import timeutils"},{"line_number":28,"context_line":"from ovsdbapp.backend.ovs_idl import event as row_event"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"from neutron.common.ovn import acl as acl_utils"},{"line_number":31,"context_line":"from neutron.common.ovn import constants as ovn_const"},{"line_number":32,"context_line":"from neutron.common.ovn import utils"},{"line_number":33,"context_line":"from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff570b3c_83af4f7b","line":30,"updated":"2020-06-05 22:05:40.000000000","message":"pep8: F401 \u0027neutron.common.ovn.acl as acl_utils\u0027 imported but unused","commit_id":"19cd35947c3f01ba6c32e2318951c0383cd25993"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py":[{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"2665a33d8ed7fc6e4d60b7f50e8804f4c338bd84","unresolved":false,"context_lines":[{"line_number":2076,"context_line":"        if self._nb_idl.get_port_group(pg_name):"},{"line_number":2077,"context_line":"            return"},{"line_number":2078,"context_line":"        # Create subnet Port Group."},{"line_number":2079,"context_line":"        ext_ids \u003d {ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name("},{"line_number":2080,"context_line":"            subnet[\u0027network_id\u0027])}"},{"line_number":2081,"context_line":"        txn.add(self._nb_idl.pg_add(pg_name, acls\u003d[], external_ids\u003dext_ids))"},{"line_number":2082,"context_line":"        # Add ACLs to this Port Group."}],"source_content_type":"text/x-python","patch_set":7,"id":"ff570b3c_b6d74f23","line":2079,"range":{"start_line":2079,"start_character":29,"end_line":2079,"end_character":56},"updated":"2020-06-04 15:44:17.000000000","message":"I think we want the subnet external id as well to ensure consistency?","commit_id":"42ca5dd6d00bf8b34eb63ac760bf68002e58f238"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"938252ca8c35ccdc1c8ea5f04e2bff3d391747bf","unresolved":false,"context_lines":[{"line_number":2076,"context_line":"        if self._nb_idl.get_port_group(pg_name):"},{"line_number":2077,"context_line":"            return"},{"line_number":2078,"context_line":"        # Create subnet Port Group."},{"line_number":2079,"context_line":"        ext_ids \u003d {ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name("},{"line_number":2080,"context_line":"            subnet[\u0027network_id\u0027])}"},{"line_number":2081,"context_line":"        txn.add(self._nb_idl.pg_add(pg_name, acls\u003d[], external_ids\u003dext_ids))"},{"line_number":2082,"context_line":"        # Add ACLs to this Port Group."}],"source_content_type":"text/x-python","patch_set":7,"id":"ff570b3c_fcffcca7","line":2079,"range":{"start_line":2079,"start_character":29,"end_line":2079,"end_character":56},"in_reply_to":"ff570b3c_b6d74f23","updated":"2020-06-06 11:45:57.000000000","message":"We can easily derive the neutron subnet id from the name of the port group. So I don\u0027t think that having that as an external id adds value; just takes memory. Locating the subnet from its network id is also very easy to do.","commit_id":"42ca5dd6d00bf8b34eb63ac760bf68002e58f238"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"30f85265896d0b98a8edf32cbadb2ac936ccd0c2","unresolved":false,"context_lines":[{"line_number":476,"context_line":"                    network_id)}))"},{"line_number":477,"context_line":"        for network_subnet_pg in network_subnet_pgs.execute(check_error\u003dTrue):"},{"line_number":478,"context_line":"            if port_uuid in network_subnet_pg.get(\u0027ports\u0027, []):"},{"line_number":479,"context_line":"                subnet_pgs.add(network_subnet_pg.get(\u0027name\u0027))"},{"line_number":480,"context_line":"        return subnet_pgs"},{"line_number":481,"context_line":""},{"line_number":482,"context_line":"    def _get_subnet_pgs_for_port(self, context, port):"}],"source_content_type":"text/x-python","patch_set":12,"id":"ff570b3c_76aa8971","line":479,"range":{"start_line":479,"start_character":31,"end_line":479,"end_character":60},"updated":"2020-06-09 21:35:07.000000000","message":"nit: If we expect this to always be there, I think this should be:\n\n network_subnet_pg[\u0027name\u0027]\n\nsince get() implies that we are purposely returning None if \u0027name\u0027 doesn\u0027t exist (and it must since it is an indexed column) and adding None to subnet_pgs doesn\u0027t make sense.","commit_id":"4a017675b1645509cf5c437ef3fc6f2914b6030d"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"f327502a7297bb641aff749ac9ee240197471cb6","unresolved":false,"context_lines":[{"line_number":476,"context_line":"                    network_id)}))"},{"line_number":477,"context_line":"        for network_subnet_pg in network_subnet_pgs.execute(check_error\u003dTrue):"},{"line_number":478,"context_line":"            if port_uuid in network_subnet_pg.get(\u0027ports\u0027, []):"},{"line_number":479,"context_line":"                subnet_pgs.add(network_subnet_pg.get(\u0027name\u0027))"},{"line_number":480,"context_line":"        return subnet_pgs"},{"line_number":481,"context_line":""},{"line_number":482,"context_line":"    def _get_subnet_pgs_for_port(self, context, port):"}],"source_content_type":"text/x-python","patch_set":12,"id":"ff570b3c_0d02130a","line":479,"range":{"start_line":479,"start_character":31,"end_line":479,"end_character":60},"in_reply_to":"ff570b3c_76aa8971","updated":"2020-06-10 10:33:43.000000000","message":"ack. Let me change that!","commit_id":"4a017675b1645509cf5c437ef3fc6f2914b6030d"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"62884f8c2defb84d4c4e6d2f2894802e17c0af5f","unresolved":false,"context_lines":[{"line_number":416,"context_line":"                    txn.add(self._nb_idl.pg_add_ports("},{"line_number":417,"context_line":"                        utils.ovn_port_group_name(sg), port_cmd))"},{"line_number":418,"context_line":"            else:"},{"line_number":419,"context_line":"                # SGs modelled as Port Groups:"},{"line_number":420,"context_line":"                acls_new \u003d ovn_acl.add_acls(self._plugin, context,"},{"line_number":421,"context_line":"                                            port, sg_cache, subnet_cache,"},{"line_number":422,"context_line":"                                            self._nb_idl)"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_1efd1b08","line":419,"range":{"start_line":419,"start_character":34,"end_line":419,"end_character":45},"updated":"2020-06-10 12:49:48.000000000","message":"Why this? I think this block is still for SGs modelled as Address sets right?\n\nI\u0027m unsure if I misled you on this one so apologies if that\u0027s the case :)","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"94ee1e5f183d3ba0a09c66333442f234d20e01c1","unresolved":false,"context_lines":[{"line_number":416,"context_line":"                    txn.add(self._nb_idl.pg_add_ports("},{"line_number":417,"context_line":"                        utils.ovn_port_group_name(sg), port_cmd))"},{"line_number":418,"context_line":"            else:"},{"line_number":419,"context_line":"                # SGs modelled as Port Groups:"},{"line_number":420,"context_line":"                acls_new \u003d ovn_acl.add_acls(self._plugin, context,"},{"line_number":421,"context_line":"                                            port, sg_cache, subnet_cache,"},{"line_number":422,"context_line":"                                            self._nb_idl)"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_ce6bee82","line":419,"range":{"start_line":419,"start_character":34,"end_line":419,"end_character":45},"in_reply_to":"ff570b3c_1efd1b08","updated":"2020-06-10 14:51:11.000000000","message":"This is straight out of https://review.opendev.org/#/c/600365/1/networking_ovn/common/ovn_client.py@318 ...\nI will revert it.","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"649ce1f6e9a99bbd16d27d623d8c23ae742a2506","unresolved":false,"context_lines":[{"line_number":470,"context_line":""},{"line_number":471,"context_line":"    def _get_ovn_subnet_pgs_for_port(self, port_uuid, network_id):"},{"line_number":472,"context_line":"        subnet_pgs \u003d set()"},{"line_number":473,"context_line":"        network_subnet_pgs \u003d self._nb_idl.db_find(\u0027Port_Group\u0027, ("},{"line_number":474,"context_line":"            \u0027external_ids\u0027, \u0027\u003d\u0027, {"},{"line_number":475,"context_line":"                ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name("},{"line_number":476,"context_line":"                    network_id)}))"},{"line_number":477,"context_line":"        for network_subnet_pg in network_subnet_pgs.execute(check_error\u003dTrue):"},{"line_number":478,"context_line":"            if port_uuid in network_subnet_pg.get(\u0027ports\u0027, []):"},{"line_number":479,"context_line":"                subnet_pgs.add(network_subnet_pg[\u0027name\u0027])"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_ab72ec47","line":476,"range":{"start_line":473,"start_character":0,"end_line":476,"end_character":34},"updated":"2020-06-10 14:19:27.000000000","message":"Can\u0027t we try to find the subnet PG in OVN using the name (pg-\u003csubnet uuid\u003e) instead of the external_ids ? The port object can be passed to this method just like it\u0027s passed to _get_subnet_pgs_for_port() (L482). \n\nAsking cause the \"name\" attribute of the table is indexable and will benefit from the latest additions in ovsdbapp that auto indexes these fields. The external_id is a json blob that can\u0027t be indexed it need to be inspected in a loop as you did here.","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"94ee1e5f183d3ba0a09c66333442f234d20e01c1","unresolved":false,"context_lines":[{"line_number":470,"context_line":""},{"line_number":471,"context_line":"    def _get_ovn_subnet_pgs_for_port(self, port_uuid, network_id):"},{"line_number":472,"context_line":"        subnet_pgs \u003d set()"},{"line_number":473,"context_line":"        network_subnet_pgs \u003d self._nb_idl.db_find(\u0027Port_Group\u0027, ("},{"line_number":474,"context_line":"            \u0027external_ids\u0027, \u0027\u003d\u0027, {"},{"line_number":475,"context_line":"                ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name("},{"line_number":476,"context_line":"                    network_id)}))"},{"line_number":477,"context_line":"        for network_subnet_pg in network_subnet_pgs.execute(check_error\u003dTrue):"},{"line_number":478,"context_line":"            if port_uuid in network_subnet_pg.get(\u0027ports\u0027, []):"},{"line_number":479,"context_line":"                subnet_pgs.add(network_subnet_pg[\u0027name\u0027])"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_2e254aac","line":476,"range":{"start_line":473,"start_character":0,"end_line":476,"end_character":34},"in_reply_to":"ff570b3c_ab72ec47","updated":"2020-06-10 14:51:11.000000000","message":"The subnet ids are not known or can have been changed for the port. The network is the only thing that we know for sure that cannot change, and that is why it is used here.\nBUT I will change this code to use the name, so this will not be an issue.","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"e06e30c0e80c74380c5ad3bdf80af052a09d371d","unresolved":false,"context_lines":[{"line_number":2060,"context_line":"                subnet[\u0027id\u0027], port_id\u003dport_id, options\u003doptions))"},{"line_number":2061,"context_line":""},{"line_number":2062,"context_line":"    def create_subnet(self, context, subnet, network):"},{"line_number":2063,"context_line":"        if subnet[\u0027enable_dhcp\u0027]:"},{"line_number":2064,"context_line":"            if subnet[\u0027ip_version\u0027] \u003d\u003d 4:"},{"line_number":2065,"context_line":"                self.update_metadata_port(context, network[\u0027id\u0027])"},{"line_number":2066,"context_line":"            self._add_subnet_dhcp_options(subnet, network)"},{"line_number":2067,"context_line":""},{"line_number":2068,"context_line":"            if self._nb_idl.is_port_groups_supported():"},{"line_number":2069,"context_line":"                with self._nb_idl.transaction(check_error\u003dTrue) as txn:"},{"line_number":2070,"context_line":"                    self._create_subnet_port_group(subnet, txn)"},{"line_number":2071,"context_line":""},{"line_number":2072,"context_line":"        db_rev.bump_revision(context, subnet, ovn_const.TYPE_SUBNETS)"},{"line_number":2073,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_79e1d1fc","line":2070,"range":{"start_line":2063,"start_character":0,"end_line":2070,"end_character":63},"updated":"2020-06-10 13:31:34.000000000","message":"I\u0027ll let Lucas confirm but if you don\u0027t consolidate these two things into the same transaction, you risk having inconsistencies. You want a Neutron subnet to map to an OVN row tuple (DHCP_Options, Port_Group). Once all\u0027s committed in OVN you\u0027ll bump the revision.\n\nYour current approach may work anyways as L2072 won\u0027t be hit if there\u0027s an error here. However, the approach followed all over the place is consolidating all the commands into a single transaction to tackle consistency:\n\nhttps://docs.openstack.org/networking-ovn/queens/contributor/design/database_consistency.html#ensure-correctness-when-updating-ovn","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"649ce1f6e9a99bbd16d27d623d8c23ae742a2506","unresolved":false,"context_lines":[{"line_number":2060,"context_line":"                subnet[\u0027id\u0027], port_id\u003dport_id, options\u003doptions))"},{"line_number":2061,"context_line":""},{"line_number":2062,"context_line":"    def create_subnet(self, context, subnet, network):"},{"line_number":2063,"context_line":"        if subnet[\u0027enable_dhcp\u0027]:"},{"line_number":2064,"context_line":"            if subnet[\u0027ip_version\u0027] \u003d\u003d 4:"},{"line_number":2065,"context_line":"                self.update_metadata_port(context, network[\u0027id\u0027])"},{"line_number":2066,"context_line":"            self._add_subnet_dhcp_options(subnet, network)"},{"line_number":2067,"context_line":""},{"line_number":2068,"context_line":"            if self._nb_idl.is_port_groups_supported():"},{"line_number":2069,"context_line":"                with self._nb_idl.transaction(check_error\u003dTrue) as txn:"},{"line_number":2070,"context_line":"                    self._create_subnet_port_group(subnet, txn)"},{"line_number":2071,"context_line":""},{"line_number":2072,"context_line":"        db_rev.bump_revision(context, subnet, ovn_const.TYPE_SUBNETS)"},{"line_number":2073,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_cbd80020","line":2070,"range":{"start_line":2063,"start_character":0,"end_line":2070,"end_character":63},"in_reply_to":"ff570b3c_79e1d1fc","updated":"2020-06-10 14:19:27.000000000","message":"Right.\n\nThe reason to have both in the same transaction is so that we can re-use the same revision number for both the subnet and the subnet Port Group. Because if both are created/updated/deleted as part of the same transaction the database guarantees that they share the same state.\n\nIf you do in two transactions you are at risk of something failing in between them leading to inconsistencies as @Daniel pointed out.","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"94ee1e5f183d3ba0a09c66333442f234d20e01c1","unresolved":false,"context_lines":[{"line_number":2060,"context_line":"                subnet[\u0027id\u0027], port_id\u003dport_id, options\u003doptions))"},{"line_number":2061,"context_line":""},{"line_number":2062,"context_line":"    def create_subnet(self, context, subnet, network):"},{"line_number":2063,"context_line":"        if subnet[\u0027enable_dhcp\u0027]:"},{"line_number":2064,"context_line":"            if subnet[\u0027ip_version\u0027] \u003d\u003d 4:"},{"line_number":2065,"context_line":"                self.update_metadata_port(context, network[\u0027id\u0027])"},{"line_number":2066,"context_line":"            self._add_subnet_dhcp_options(subnet, network)"},{"line_number":2067,"context_line":""},{"line_number":2068,"context_line":"            if self._nb_idl.is_port_groups_supported():"},{"line_number":2069,"context_line":"                with self._nb_idl.transaction(check_error\u003dTrue) as txn:"},{"line_number":2070,"context_line":"                    self._create_subnet_port_group(subnet, txn)"},{"line_number":2071,"context_line":""},{"line_number":2072,"context_line":"        db_rev.bump_revision(context, subnet, ovn_const.TYPE_SUBNETS)"},{"line_number":2073,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_cb4da0c5","line":2070,"range":{"start_line":2063,"start_character":0,"end_line":2070,"end_character":63},"in_reply_to":"ff570b3c_cbd80020","updated":"2020-06-10 14:51:11.000000000","message":"ack! will address this.","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"62884f8c2defb84d4c4e6d2f2894802e17c0af5f","unresolved":false,"context_lines":[{"line_number":2076,"context_line":"        if self._nb_idl.get_port_group(pg_name):"},{"line_number":2077,"context_line":"            return"},{"line_number":2078,"context_line":"        # Create subnet Port Group."},{"line_number":2079,"context_line":"        ext_ids \u003d {ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name("},{"line_number":2080,"context_line":"            subnet[\u0027network_id\u0027])}"},{"line_number":2081,"context_line":"        txn.add(self._nb_idl.pg_add(pg_name, acls\u003d[], external_ids\u003dext_ids))"},{"line_number":2082,"context_line":"        # Add ACLs to this Port Group."},{"line_number":2083,"context_line":"        acls \u003d ovn_acl.add_acls_for_subnet_port_group(pg_name, subnet)"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_9eac8bef","line":2080,"range":{"start_line":2079,"start_character":0,"end_line":2080,"end_character":34},"updated":"2020-06-10 12:49:48.000000000","message":"What\u0027s the purpose of the network ext id?\nAs the PG is mapping a neutron subnet shouldn\u0027t it be referencing that?\nAlso for the sync part it\u0027s easier to pull all the subnets that require an OVN subnet port group (ie. all subnets with dhcp enabled) and decide if they\u0027re present by looking at the uuids (pull set with the first, set with the second and if they\u0027re equal, then you\u0027re not missing anything). Otherwise, how\u0027d you be sure that this will be true?","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"94ee1e5f183d3ba0a09c66333442f234d20e01c1","unresolved":false,"context_lines":[{"line_number":2076,"context_line":"        if self._nb_idl.get_port_group(pg_name):"},{"line_number":2077,"context_line":"            return"},{"line_number":2078,"context_line":"        # Create subnet Port Group."},{"line_number":2079,"context_line":"        ext_ids \u003d {ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name("},{"line_number":2080,"context_line":"            subnet[\u0027network_id\u0027])}"},{"line_number":2081,"context_line":"        txn.add(self._nb_idl.pg_add(pg_name, acls\u003d[], external_ids\u003dext_ids))"},{"line_number":2082,"context_line":"        # Add ACLs to this Port Group."},{"line_number":2083,"context_line":"        acls \u003d ovn_acl.add_acls_for_subnet_port_group(pg_name, subnet)"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_6bfe1439","line":2080,"range":{"start_line":2079,"start_character":0,"end_line":2080,"end_character":34},"in_reply_to":"ff570b3c_9eac8bef","updated":"2020-06-10 14:51:11.000000000","message":"I like the idea of using the name of the pg to reflect that it is used as subnet pg (vs sg pg). With that external_ids will not be used at all anymore. Will address this.","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"1134f4598c3c2aed20d2034111b669aa1c76574a","unresolved":false,"context_lines":[{"line_number":2076,"context_line":"        if self._nb_idl.get_port_group(pg_name):"},{"line_number":2077,"context_line":"            return"},{"line_number":2078,"context_line":"        # Create subnet Port Group."},{"line_number":2079,"context_line":"        ext_ids \u003d {ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY: utils.ovn_name("},{"line_number":2080,"context_line":"            subnet[\u0027network_id\u0027])}"},{"line_number":2081,"context_line":"        txn.add(self._nb_idl.pg_add(pg_name, acls\u003d[], external_ids\u003dext_ids))"},{"line_number":2082,"context_line":"        # Add ACLs to this Port Group."},{"line_number":2083,"context_line":"        acls \u003d ovn_acl.add_acls_for_subnet_port_group(pg_name, subnet)"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_cb5595fc","line":2080,"range":{"start_line":2079,"start_character":0,"end_line":2080,"end_character":34},"in_reply_to":"ff570b3c_9eac8bef","updated":"2020-06-10 22:14:42.000000000","message":"Okay... I did a really bad job in explaining why network id is needed as external id, so let me try again. ;)\n\nWhen we are in the update_port() codepath -- line 596 above -- we need to locate all the \u0027old\u0027 ovn subnet_pgs that may have the logical port listed under \u0027ports\u0027 INCLUDING the subnets that the \u0027updated\u0027 port is no longer part of. Since a port\u0027s network_id cannot be modified, the idea is that we can narrow down the search to only the subnet pgs that are part of that network_id.\nHaving said that, I think I will just iterate through all the subnet pgs for now and save on the memory it takes to have the external_id.","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"279df9104df7ade41fc5a92b85ed4e0dd0052c6e","unresolved":false,"context_lines":[{"line_number":471,"context_line":""},{"line_number":472,"context_line":"    def _get_ovn_subnet_pgs_for_port(self, port_uuid):"},{"line_number":473,"context_line":"        subnet_pgs \u003d set()"},{"line_number":474,"context_line":"        network_subnet_pgs \u003d self._nb_idl.db_find(\u0027Port_Group\u0027)"},{"line_number":475,"context_line":"        for network_subnet_pg in network_subnet_pgs.execute(check_error\u003dTrue):"},{"line_number":476,"context_line":"            sg_name \u003d network_subnet_pg[\u0027name\u0027]"},{"line_number":477,"context_line":"            if (utils.is_ovn_subnet_port_group_name(sg_name) and"},{"line_number":478,"context_line":"                    port_uuid in network_subnet_pg[\u0027ports\u0027]):"},{"line_number":479,"context_line":"                subnet_pgs.add(sg_name)"},{"line_number":480,"context_line":"        return subnet_pgs"},{"line_number":481,"context_line":""},{"line_number":482,"context_line":"    def _get_subnet_pgs_for_port(self, context, port):"}],"source_content_type":"text/x-python","patch_set":16,"id":"ff570b3c_eca34cff","line":479,"range":{"start_line":474,"start_character":0,"end_line":479,"end_character":39},"updated":"2020-06-11 09:18:25.000000000","message":"Just thinking out loud:\n\nSorry for the back and forth here, it\u0027s just that the update_port() method is already slow as-is so I\u0027m thinking about ways that we could optimize it.\n\nLooping over all entries in the Port_Group table works but it\u0027s not optimal, I was wondering if we could have the subnets id\u0027s in the Logical_Switch_Port entry as we do for the router ports [0].\n\nThen in this method we could take advantage of the indexing in ovsdbapp by using lookup to find the PG(s):\n\nfor subnet_id in utils.get_subnets_from_ovn_port(port):\n    try:\n        pg \u003d self._nb_idl.lookup(\u0027Port_Group\u0027, utils.ovn_subnet_port_group_name(subnet_id))\n        subnet_pgs.append(pg.name)\n     except RowNotFound:\n        continue\n\nThis is just an idea.... Maybe we can leave the code as-is now and leave it as a TODO to optimize later. Let\u0027s see what reviewers think.\n\n[0] https://github.com/openstack/neutron/blob/b69e8892d91d2d81c0ca2fc297dfd087517ae730/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py#L1300-L1301","commit_id":"340019235e8c773d20e3bcf24dce929775241ba3"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"89041cb1eec2e47ce8ad854c2a7fde8765724ac2","unresolved":false,"context_lines":[{"line_number":471,"context_line":""},{"line_number":472,"context_line":"    def _get_ovn_subnet_pgs_for_port(self, port_uuid):"},{"line_number":473,"context_line":"        subnet_pgs \u003d set()"},{"line_number":474,"context_line":"        network_subnet_pgs \u003d self._nb_idl.db_find(\u0027Port_Group\u0027)"},{"line_number":475,"context_line":"        for network_subnet_pg in network_subnet_pgs.execute(check_error\u003dTrue):"},{"line_number":476,"context_line":"            sg_name \u003d network_subnet_pg[\u0027name\u0027]"},{"line_number":477,"context_line":"            if (utils.is_ovn_subnet_port_group_name(sg_name) and"},{"line_number":478,"context_line":"                    port_uuid in network_subnet_pg[\u0027ports\u0027]):"},{"line_number":479,"context_line":"                subnet_pgs.add(sg_name)"},{"line_number":480,"context_line":"        return subnet_pgs"},{"line_number":481,"context_line":""},{"line_number":482,"context_line":"    def _get_subnet_pgs_for_port(self, context, port):"}],"source_content_type":"text/x-python","patch_set":16,"id":"ff570b3c_7db7e83b","line":479,"range":{"start_line":474,"start_character":0,"end_line":479,"end_character":39},"in_reply_to":"ff570b3c_1d3a2cc3","updated":"2020-06-11 12:24:36.000000000","message":"If you use a name like \u0027pg_subnet_uuid\u0027 then you can fetch all PGs whose name starts with \u0027pg_subnet_\u0027. You\u0027ll then fetch the uuids and match against Neutron subnet uuids which have dhcp enabled.\n\nThe delta is what requires fixing in the ovn db sync script either removal or add it from/to OVN.","commit_id":"340019235e8c773d20e3bcf24dce929775241ba3"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"ebd416bc517a9b8be27fbb209823f4bb3f5a22d9","unresolved":false,"context_lines":[{"line_number":471,"context_line":""},{"line_number":472,"context_line":"    def _get_ovn_subnet_pgs_for_port(self, port_uuid):"},{"line_number":473,"context_line":"        subnet_pgs \u003d set()"},{"line_number":474,"context_line":"        network_subnet_pgs \u003d self._nb_idl.db_find(\u0027Port_Group\u0027)"},{"line_number":475,"context_line":"        for network_subnet_pg in network_subnet_pgs.execute(check_error\u003dTrue):"},{"line_number":476,"context_line":"            sg_name \u003d network_subnet_pg[\u0027name\u0027]"},{"line_number":477,"context_line":"            if (utils.is_ovn_subnet_port_group_name(sg_name) and"},{"line_number":478,"context_line":"                    port_uuid in network_subnet_pg[\u0027ports\u0027]):"},{"line_number":479,"context_line":"                subnet_pgs.add(sg_name)"},{"line_number":480,"context_line":"        return subnet_pgs"},{"line_number":481,"context_line":""},{"line_number":482,"context_line":"    def _get_subnet_pgs_for_port(self, context, port):"}],"source_content_type":"text/x-python","patch_set":16,"id":"ff570b3c_44db2420","line":479,"range":{"start_line":474,"start_character":0,"end_line":479,"end_character":39},"in_reply_to":"ff570b3c_5df7845a","updated":"2020-06-11 19:23:46.000000000","message":"Yes! TY Daniel and Lucas for setting me straight.\nAs we discussed offline, I\u0027m taking an easier approach here and assume that when subnet is detached from network it is: 1) about to be deleted or 2) it does not matter that port uuid is part the pg, because the port is not part of the subnet. With that, I will error on the side of performance here and just focus on the port updates where new subnets are attached to an existing port. I will add a NOTE in the code explaining this decision.","commit_id":"340019235e8c773d20e3bcf24dce929775241ba3"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"6c23236a088940950107ec8f11856256940667c7","unresolved":false,"context_lines":[{"line_number":471,"context_line":""},{"line_number":472,"context_line":"    def _get_ovn_subnet_pgs_for_port(self, port_uuid):"},{"line_number":473,"context_line":"        subnet_pgs \u003d set()"},{"line_number":474,"context_line":"        network_subnet_pgs \u003d self._nb_idl.db_find(\u0027Port_Group\u0027)"},{"line_number":475,"context_line":"        for network_subnet_pg in network_subnet_pgs.execute(check_error\u003dTrue):"},{"line_number":476,"context_line":"            sg_name \u003d network_subnet_pg[\u0027name\u0027]"},{"line_number":477,"context_line":"            if (utils.is_ovn_subnet_port_group_name(sg_name) and"},{"line_number":478,"context_line":"                    port_uuid in network_subnet_pg[\u0027ports\u0027]):"},{"line_number":479,"context_line":"                subnet_pgs.add(sg_name)"},{"line_number":480,"context_line":"        return subnet_pgs"},{"line_number":481,"context_line":""},{"line_number":482,"context_line":"    def _get_subnet_pgs_for_port(self, context, port):"}],"source_content_type":"text/x-python","patch_set":16,"id":"ff570b3c_fd077817","line":479,"range":{"start_line":474,"start_character":0,"end_line":479,"end_character":39},"in_reply_to":"ff570b3c_eca34cff","updated":"2020-06-11 12:18:52.000000000","message":"No, that will not work.\n\nThat was the purpose of the external id for networking id.\n\nAs I mentioned before, we cannot rely on the subnet id because that is exactly what may have been removed from the neutron db. We could just punt on this corner case and let eh subnet pg go stale wrt the ports that are in the pg? That would make it faster. ;)","commit_id":"340019235e8c773d20e3bcf24dce929775241ba3"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"83e9786138c29db89eb66c184be5a29561cabf7a","unresolved":false,"context_lines":[{"line_number":471,"context_line":""},{"line_number":472,"context_line":"    def _get_ovn_subnet_pgs_for_port(self, port_uuid):"},{"line_number":473,"context_line":"        subnet_pgs \u003d set()"},{"line_number":474,"context_line":"        network_subnet_pgs \u003d self._nb_idl.db_find(\u0027Port_Group\u0027)"},{"line_number":475,"context_line":"        for network_subnet_pg in network_subnet_pgs.execute(check_error\u003dTrue):"},{"line_number":476,"context_line":"            sg_name \u003d network_subnet_pg[\u0027name\u0027]"},{"line_number":477,"context_line":"            if (utils.is_ovn_subnet_port_group_name(sg_name) and"},{"line_number":478,"context_line":"                    port_uuid in network_subnet_pg[\u0027ports\u0027]):"},{"line_number":479,"context_line":"                subnet_pgs.add(sg_name)"},{"line_number":480,"context_line":"        return subnet_pgs"},{"line_number":481,"context_line":""},{"line_number":482,"context_line":"    def _get_subnet_pgs_for_port(self, context, port):"}],"source_content_type":"text/x-python","patch_set":16,"id":"ff570b3c_5df7845a","line":479,"range":{"start_line":474,"start_character":0,"end_line":479,"end_character":39},"in_reply_to":"ff570b3c_fd077817","updated":"2020-06-11 12:46:05.000000000","message":"For example, you can find the subnets that neutron port object by looking into its \"fixed_ips\" attribute (like you did on L484) and save it into the external_ids column for the Logical_Switch_Port entry in OVN NB DB.\n\nEvery time a port is updated, this method computes the differences to see if a subnet has been added to the port right ? \n\nWith this approach, you can get the subnet ids from the Neutron port object(via fixed_ips) that is passed to the update_port() method; that represents the **current** state in neutron, and, in the same method you compare those IDs with the subnet ids we have in the OVN database which represent the **old** state, remember this is before we commit the change to the OVN DB.\n\nNow if an ID is the Neutron port but not in the related Logical_Switch_Port entry in the OVN NB DB, it means that this port has been added to a new subnet. Similar when a port is removed from a subnet, if the ID is in the OVN DB but not in the Neutron port object it means that the port has been removed from that subnet.\n\nMakes sense ?","commit_id":"340019235e8c773d20e3bcf24dce929775241ba3"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"3aaeb99282f2d8a4ffbcc473da37cfc1fa93a396","unresolved":false,"context_lines":[{"line_number":471,"context_line":""},{"line_number":472,"context_line":"    def _get_ovn_subnet_pgs_for_port(self, port_uuid):"},{"line_number":473,"context_line":"        subnet_pgs \u003d set()"},{"line_number":474,"context_line":"        network_subnet_pgs \u003d self._nb_idl.db_find(\u0027Port_Group\u0027)"},{"line_number":475,"context_line":"        for network_subnet_pg in network_subnet_pgs.execute(check_error\u003dTrue):"},{"line_number":476,"context_line":"            sg_name \u003d network_subnet_pg[\u0027name\u0027]"},{"line_number":477,"context_line":"            if (utils.is_ovn_subnet_port_group_name(sg_name) and"},{"line_number":478,"context_line":"                    port_uuid in network_subnet_pg[\u0027ports\u0027]):"},{"line_number":479,"context_line":"                subnet_pgs.add(sg_name)"},{"line_number":480,"context_line":"        return subnet_pgs"},{"line_number":481,"context_line":""},{"line_number":482,"context_line":"    def _get_subnet_pgs_for_port(self, context, port):"}],"source_content_type":"text/x-python","patch_set":16,"id":"ff570b3c_1d3a2cc3","line":479,"range":{"start_line":474,"start_character":0,"end_line":479,"end_character":39},"in_reply_to":"ff570b3c_fd077817","updated":"2020-06-11 12:21:16.000000000","message":"I\u0027m not sure I understand your reply Flavio.\nWouldn\u0027t every other object suffer from the same thing then? ie. a Neutron network removed from the DB and all Logical Switches referencing its external uuid (same thing would go for a security group rule and ACL for example).","commit_id":"340019235e8c773d20e3bcf24dce929775241ba3"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"279df9104df7ade41fc5a92b85ed4e0dd0052c6e","unresolved":false,"context_lines":[{"line_number":1832,"context_line":"            utils.get_revision_number(subnet, ovn_const.TYPE_SUBNETS))}"},{"line_number":1833,"context_line":"        ovn_dhcp_options[\u0027external_ids\u0027].update(rev_num)"},{"line_number":1834,"context_line":""},{"line_number":1835,"context_line":"        if txn:"},{"line_number":1836,"context_line":"            txn.add(self._nb_idl.add_dhcp_options("},{"line_number":1837,"context_line":"                subnet[\u0027id\u0027], **ovn_dhcp_options))"},{"line_number":1838,"context_line":"        else:"},{"line_number":1839,"context_line":"            with self._nb_idl.transaction(check_error\u003dTrue) as txn_local:"},{"line_number":1840,"context_line":"                txn_local.add(self._nb_idl.add_dhcp_options("},{"line_number":1841,"context_line":"                    subnet[\u0027id\u0027], **ovn_dhcp_options))"},{"line_number":1842,"context_line":""},{"line_number":1843,"context_line":"    def _get_ovn_dhcp_options(self, subnet, network, server_mac\u003dNone):"},{"line_number":1844,"context_line":"        external_ids \u003d {"}],"source_content_type":"text/x-python","patch_set":16,"id":"ff570b3c_8c29f08a","line":1841,"range":{"start_line":1835,"start_character":0,"end_line":1841,"end_character":54},"updated":"2020-06-11 09:18:25.000000000","message":"We have a wrapper for this:\n\nself._transaction([self._nb_idl.add_dhcp_options(subnet[\u0027id\u0027], **ovn_dhcp_options)], txn\u003dtxn)","commit_id":"340019235e8c773d20e3bcf24dce929775241ba3"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"ebd416bc517a9b8be27fbb209823f4bb3f5a22d9","unresolved":false,"context_lines":[{"line_number":1832,"context_line":"            utils.get_revision_number(subnet, ovn_const.TYPE_SUBNETS))}"},{"line_number":1833,"context_line":"        ovn_dhcp_options[\u0027external_ids\u0027].update(rev_num)"},{"line_number":1834,"context_line":""},{"line_number":1835,"context_line":"        if txn:"},{"line_number":1836,"context_line":"            txn.add(self._nb_idl.add_dhcp_options("},{"line_number":1837,"context_line":"                subnet[\u0027id\u0027], **ovn_dhcp_options))"},{"line_number":1838,"context_line":"        else:"},{"line_number":1839,"context_line":"            with self._nb_idl.transaction(check_error\u003dTrue) as txn_local:"},{"line_number":1840,"context_line":"                txn_local.add(self._nb_idl.add_dhcp_options("},{"line_number":1841,"context_line":"                    subnet[\u0027id\u0027], **ovn_dhcp_options))"},{"line_number":1842,"context_line":""},{"line_number":1843,"context_line":"    def _get_ovn_dhcp_options(self, subnet, network, server_mac\u003dNone):"},{"line_number":1844,"context_line":"        external_ids \u003d {"}],"source_content_type":"text/x-python","patch_set":16,"id":"ff570b3c_840c1c95","line":1841,"range":{"start_line":1835,"start_character":0,"end_line":1841,"end_character":54},"in_reply_to":"ff570b3c_8c29f08a","updated":"2020-06-11 19:23:46.000000000","message":"Nice! Done.","commit_id":"340019235e8c773d20e3bcf24dce929775241ba3"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"c51932f45240cac9624bd9a84a9134821e0d37d0","unresolved":false,"context_lines":[{"line_number":2002,"context_line":"            self._create_subnet_port_group(subnet, txn)"},{"line_number":2003,"context_line":"        elif subnet[\u0027enable_dhcp\u0027] and ovn_subnet:"},{"line_number":2004,"context_line":"            self._update_subnet_dhcp_options(subnet, network, txn)"},{"line_number":2005,"context_line":"            # subnet pg is idem-potent, no harm in calling it again"},{"line_number":2006,"context_line":"            if utils.is_subnet_pg_required(subnet):"},{"line_number":2007,"context_line":"                self._create_subnet_port_group(subnet, txn)"},{"line_number":2008,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":20,"id":"bf51134e_142230a5","line":2005,"range":{"start_line":2005,"start_character":11,"end_line":2005,"end_character":67},"updated":"2020-06-16 09:56:12.000000000","message":"It\u0027s idem-potent but adding commands to the transaction. Could we avoid that maybe?","commit_id":"888bbae055a48844a0badaf361605fe127a81b22"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"ef018f9d1a260f7434912ab5855cea2c84bf36e8","unresolved":false,"context_lines":[{"line_number":2002,"context_line":"            self._create_subnet_port_group(subnet, txn)"},{"line_number":2003,"context_line":"        elif subnet[\u0027enable_dhcp\u0027] and ovn_subnet:"},{"line_number":2004,"context_line":"            self._update_subnet_dhcp_options(subnet, network, txn)"},{"line_number":2005,"context_line":"            # subnet pg is idem-potent, no harm in calling it again"},{"line_number":2006,"context_line":"            if utils.is_subnet_pg_required(subnet):"},{"line_number":2007,"context_line":"                self._create_subnet_port_group(subnet, txn)"},{"line_number":2008,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":20,"id":"bf51134e_14dd1042","line":2005,"range":{"start_line":2005,"start_character":11,"end_line":2005,"end_character":67},"in_reply_to":"bf51134e_142230a5","updated":"2020-06-16 10:05:33.000000000","message":"Yeah, the add already avoids it, but I can make the delete do it as well. Will do.","commit_id":"888bbae055a48844a0badaf361605fe127a81b22"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"8eff061e3f08961eb87af80c20370b96e16b336b","unresolved":false,"context_lines":[{"line_number":2002,"context_line":"            self._create_subnet_port_group(subnet, txn)"},{"line_number":2003,"context_line":"        elif subnet[\u0027enable_dhcp\u0027] and ovn_subnet:"},{"line_number":2004,"context_line":"            self._update_subnet_dhcp_options(subnet, network, txn)"},{"line_number":2005,"context_line":"            # subnet pg is idem-potent, no harm in calling it again"},{"line_number":2006,"context_line":"            if utils.is_subnet_pg_required(subnet):"},{"line_number":2007,"context_line":"                self._create_subnet_port_group(subnet, txn)"},{"line_number":2008,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":20,"id":"bf51134e_6912b15a","line":2005,"range":{"start_line":2005,"start_character":11,"end_line":2005,"end_character":67},"in_reply_to":"bf51134e_14dd1042","updated":"2020-06-16 19:32:05.000000000","message":"Done","commit_id":"888bbae055a48844a0badaf361605fe127a81b22"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py":[{"author":{"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]},"tag":"autogenerated:zuul:check","change_message_id":"74f28f8d9f86f28422efb498a66309bb30a8f6d9","unresolved":false,"context_lines":[{"line_number":247,"context_line":"                if subnet_ids_for_subnet_pg:"},{"line_number":248,"context_line":"                    db_ports_for_subnet_pg \u003d ["},{"line_number":249,"context_line":"                        port for port in db_ports if"},{"line_number":250,"context_line":"                            not utils.is_lsp_ignored("},{"line_number":251,"context_line":"                                port) and not utils.is_lsp_trusted("},{"line_number":252,"context_line":"                                port) and utils.is_port_security_enabled("},{"line_number":253,"context_line":"                                port)]"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff570b3c_63b2fbe6","line":250,"updated":"2020-06-05 22:05:40.000000000","message":"pep8: E131 continuation line unaligned for hanging indent","commit_id":"19cd35947c3f01ba6c32e2318951c0383cd25993"},{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"30f85265896d0b98a8edf32cbadb2ac936ccd0c2","unresolved":false,"context_lines":[{"line_number":1175,"context_line":"            txn.add(self.ovn_api.pg_add_ports(pg_name, ports_ids))"},{"line_number":1176,"context_line":""},{"line_number":1177,"context_line":"    def _create_subnets_port_group_ports(self, subnet_ids, db_ports, txn):"},{"line_number":1178,"context_line":"        subnet_ports \u003d {subnet_id: [] for subnet_id in subnet_ids}"},{"line_number":1179,"context_line":"        # Add ports to the relevant subnet Port Groups, after collecting"},{"line_number":1180,"context_line":"        # the ports that go to each one of them."},{"line_number":1181,"context_line":"        for port in db_ports:"}],"source_content_type":"text/x-python","patch_set":12,"id":"ff570b3c_b19d4bbd","line":1178,"updated":"2020-06-09 21:35:07.000000000","message":"nit:\n\ncould avoid iterating here by using collections.defaultdict\n\n subnet_ports \u003d defaultdict(list)\n\nthen in line 1184 doing:\n\n if subnet_id in subnet_ids:\n\nand passing in the subnet_ids as a set instead of a list in line 258 (since the call below uses .keys() which acts like a set already).","commit_id":"4a017675b1645509cf5c437ef3fc6f2914b6030d"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"94dabad957f7cec0329a11fa490a4b64ead11a92","unresolved":false,"context_lines":[{"line_number":1175,"context_line":"            txn.add(self.ovn_api.pg_add_ports(pg_name, ports_ids))"},{"line_number":1176,"context_line":""},{"line_number":1177,"context_line":"    def _create_subnets_port_group_ports(self, subnet_ids, db_ports, txn):"},{"line_number":1178,"context_line":"        subnet_ports \u003d {subnet_id: [] for subnet_id in subnet_ids}"},{"line_number":1179,"context_line":"        # Add ports to the relevant subnet Port Groups, after collecting"},{"line_number":1180,"context_line":"        # the ports that go to each one of them."},{"line_number":1181,"context_line":"        for port in db_ports:"}],"source_content_type":"text/x-python","patch_set":12,"id":"ff570b3c_98dae376","line":1178,"in_reply_to":"ff570b3c_b19d4bbd","updated":"2020-06-10 10:37:01.000000000","message":"Done","commit_id":"4a017675b1645509cf5c437ef3fc6f2914b6030d"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"62884f8c2defb84d4c4e6d2f2894802e17c0af5f","unresolved":false,"context_lines":[{"line_number":249,"context_line":"                    # Add ports to the subnet port group. Only add those that"},{"line_number":250,"context_line":"                    # already exists in OVN. The rest will be added during the"},{"line_number":251,"context_line":"                    # ports sync operation later."},{"line_number":252,"context_line":"                    db_ports_for_subnet_pg \u003d \\"},{"line_number":253,"context_line":"                        [port for port in db_ports if"},{"line_number":254,"context_line":"                         port[\u0027id\u0027] in ovn_ports and not"},{"line_number":255,"context_line":"                         utils.is_lsp_ignored(port) and not"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_9e3e4bb7","line":252,"range":{"start_line":252,"start_character":44,"end_line":252,"end_character":46},"updated":"2020-06-10 12:49:48.000000000","message":"nit: parenthesis over backslash for line continuation [0]. It can perhaps be written without any of them:?\n                                      \n\n[0] https://docs.openstack.org/hacking/latest/user/hacking.html#general","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"94ee1e5f183d3ba0a09c66333442f234d20e01c1","unresolved":false,"context_lines":[{"line_number":249,"context_line":"                    # Add ports to the subnet port group. Only add those that"},{"line_number":250,"context_line":"                    # already exists in OVN. The rest will be added during the"},{"line_number":251,"context_line":"                    # ports sync operation later."},{"line_number":252,"context_line":"                    db_ports_for_subnet_pg \u003d \\"},{"line_number":253,"context_line":"                        [port for port in db_ports if"},{"line_number":254,"context_line":"                         port[\u0027id\u0027] in ovn_ports and not"},{"line_number":255,"context_line":"                         utils.is_lsp_ignored(port) and not"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_0e8de629","line":252,"range":{"start_line":252,"start_character":44,"end_line":252,"end_character":46},"in_reply_to":"ff570b3c_9e3e4bb7","updated":"2020-06-10 14:51:11.000000000","message":"Ack! let me change that.","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"62884f8c2defb84d4c4e6d2f2894802e17c0af5f","unresolved":false,"context_lines":[{"line_number":332,"context_line":"            if skip_subnet_pgs:"},{"line_number":333,"context_line":"                # If asked, identify and skip subnet pgs by looking at"},{"line_number":334,"context_line":"                # well known external_id key."},{"line_number":335,"context_line":"                pg_ext_ids \u003d getattr(pg, \u0027external_ids\u0027, {})"},{"line_number":336,"context_line":"                if ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY in pg_ext_ids:"},{"line_number":337,"context_line":"                    continue"},{"line_number":338,"context_line":"            acls \u003d getattr(pg, \u0027acls\u0027, [])"},{"line_number":339,"context_line":"            for acl in acls:"},{"line_number":340,"context_line":"                acl_string \u003d {}"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_be686f87","line":337,"range":{"start_line":335,"start_character":0,"end_line":337,"end_character":28},"updated":"2020-06-10 12:49:48.000000000","message":"If I understood this right, you\u0027re using an external id to differentiate between a SG PG and a subnet PG. Couldn\u0027t we just add a prefix to the name and use .startswith() ? That\u0027ll save space and txns as the ext uuid gets propagated by northd to the SB database.\n\nIf you don\u0027t need any external uuid for the sync part, I\u0027d try to avoid them in this case.","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"94ee1e5f183d3ba0a09c66333442f234d20e01c1","unresolved":false,"context_lines":[{"line_number":332,"context_line":"            if skip_subnet_pgs:"},{"line_number":333,"context_line":"                # If asked, identify and skip subnet pgs by looking at"},{"line_number":334,"context_line":"                # well known external_id key."},{"line_number":335,"context_line":"                pg_ext_ids \u003d getattr(pg, \u0027external_ids\u0027, {})"},{"line_number":336,"context_line":"                if ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY in pg_ext_ids:"},{"line_number":337,"context_line":"                    continue"},{"line_number":338,"context_line":"            acls \u003d getattr(pg, \u0027acls\u0027, [])"},{"line_number":339,"context_line":"            for acl in acls:"},{"line_number":340,"context_line":"                acl_string \u003d {}"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff570b3c_4ee51edc","line":337,"range":{"start_line":335,"start_character":0,"end_line":337,"end_character":28},"in_reply_to":"ff570b3c_be686f87","updated":"2020-06-10 14:51:11.000000000","message":"Yes! I will change the name.\n\nI also left a comment in ovn_client.py that explains why we needed network_id in to know what changed on port update, but pg_name will be okay to handle that too.","commit_id":"c5e3ed6164aa4f57d67e26ac501af2f73a045f90"},{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"c51932f45240cac9624bd9a84a9134821e0d37d0","unresolved":false,"context_lines":[{"line_number":166,"context_line":"                pg_name \u003d utils.ovn_port_group_name(sg[\u0027id\u0027])"},{"line_number":167,"context_line":"                neutron_pgs.add(pg_name)"},{"line_number":168,"context_line":"                neutron_sgs[pg_name] \u003d sg[\u0027id\u0027]"},{"line_number":169,"context_line":"            for subnet in self.core_plugin.get_subnets("},{"line_number":170,"context_line":"                    ctx, filters\u003d{\u0027enable_dhcp\u0027: [True]}):"},{"line_number":171,"context_line":"                if not utils.is_subnet_pg_required(subnet):"},{"line_number":172,"context_line":"                    continue"},{"line_number":173,"context_line":"                pg_name \u003d utils.ovn_subnet_port_group_name(subnet[\u0027id\u0027])"},{"line_number":174,"context_line":"                neutron_pgs.add(pg_name)"},{"line_number":175,"context_line":"                neutron_subnet_pgs[pg_name] \u003d subnet"},{"line_number":176,"context_line":"            neutron_pgs.add(ovn_const.OVN_DROP_PORT_GROUP_NAME)"},{"line_number":177,"context_line":""},{"line_number":178,"context_line":"        ovn_pgs \u003d set()"}],"source_content_type":"text/x-python","patch_set":20,"id":"bf51134e_d4d1d859","line":175,"range":{"start_line":169,"start_character":12,"end_line":175,"end_character":52},"updated":"2020-06-16 09:56:12.000000000","message":"This seems like the natural place to sync the port groups but I wonder if there\u0027s some benefit in moving this code to \u0027sync_networks_ports_and_dhcp_opts\u0027 in order to avoid pulling *all* the subnets twice.\n\nOr pulling them before and passing the list to all the functions that need it.\nCan be a follow up though if you think it\u0027s worthy","commit_id":"888bbae055a48844a0badaf361605fe127a81b22"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"ef018f9d1a260f7434912ab5855cea2c84bf36e8","unresolved":false,"context_lines":[{"line_number":166,"context_line":"                pg_name \u003d utils.ovn_port_group_name(sg[\u0027id\u0027])"},{"line_number":167,"context_line":"                neutron_pgs.add(pg_name)"},{"line_number":168,"context_line":"                neutron_sgs[pg_name] \u003d sg[\u0027id\u0027]"},{"line_number":169,"context_line":"            for subnet in self.core_plugin.get_subnets("},{"line_number":170,"context_line":"                    ctx, filters\u003d{\u0027enable_dhcp\u0027: [True]}):"},{"line_number":171,"context_line":"                if not utils.is_subnet_pg_required(subnet):"},{"line_number":172,"context_line":"                    continue"},{"line_number":173,"context_line":"                pg_name \u003d utils.ovn_subnet_port_group_name(subnet[\u0027id\u0027])"},{"line_number":174,"context_line":"                neutron_pgs.add(pg_name)"},{"line_number":175,"context_line":"                neutron_subnet_pgs[pg_name] \u003d subnet"},{"line_number":176,"context_line":"            neutron_pgs.add(ovn_const.OVN_DROP_PORT_GROUP_NAME)"},{"line_number":177,"context_line":""},{"line_number":178,"context_line":"        ovn_pgs \u003d set()"}],"source_content_type":"text/x-python","patch_set":20,"id":"bf51134e_94f000cc","line":175,"range":{"start_line":169,"start_character":12,"end_line":175,"end_character":52},"in_reply_to":"bf51134e_d4d1d859","updated":"2020-06-16 10:05:33.000000000","message":"Good point. Will do.","commit_id":"888bbae055a48844a0badaf361605fe127a81b22"}],"neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py":[{"author":{"_account_id":5756,"name":"Terry Wilson","email":"twilson@redhat.com","username":"otherwiseguy"},"change_message_id":"30f85265896d0b98a8edf32cbadb2ac936ccd0c2","unresolved":false,"context_lines":[{"line_number":1071,"context_line":"            return"},{"line_number":1072,"context_line":""},{"line_number":1073,"context_line":"        observed_subnet_pgs_rows \u003d []"},{"line_number":1074,"context_line":"        for row in self.nb_api.tables[\u0027Port_Group\u0027].rows.values():"},{"line_number":1075,"context_line":"            ids \u003d dict(row.external_ids)"},{"line_number":1076,"context_line":"            # Identify  subnet pgs by looking for well known external_id key."},{"line_number":1077,"context_line":"            if not ids.get(ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY):"}],"source_content_type":"text/x-python","patch_set":12,"id":"ff570b3c_71ebb3db","line":1074,"range":{"start_line":1074,"start_character":12,"end_line":1074,"end_character":65},"updated":"2020-06-09 21:35:07.000000000","message":"Theoretically directly accessing the table.rows.values() is directly accessing backend ovsdbapp stuff which should only be done in Impl classes--though there is only one backend right now and there\u0027s quite a lot that actually does this all over the code--so it\u0027s probably a lost cause at this point :). nb_api.db_list_rows(\u0027Port_Group\u0027) would be the technically kosher thing to do.","commit_id":"4a017675b1645509cf5c437ef3fc6f2914b6030d"},{"author":{"_account_id":11952,"name":"Flavio Fernandes","email":"flavio@flaviof.com","username":"ffernand"},"change_message_id":"f327502a7297bb641aff749ac9ee240197471cb6","unresolved":false,"context_lines":[{"line_number":1071,"context_line":"            return"},{"line_number":1072,"context_line":""},{"line_number":1073,"context_line":"        observed_subnet_pgs_rows \u003d []"},{"line_number":1074,"context_line":"        for row in self.nb_api.tables[\u0027Port_Group\u0027].rows.values():"},{"line_number":1075,"context_line":"            ids \u003d dict(row.external_ids)"},{"line_number":1076,"context_line":"            # Identify  subnet pgs by looking for well known external_id key."},{"line_number":1077,"context_line":"            if not ids.get(ovn_const.OVN_NETWORK_NAME_EXT_ID_KEY):"}],"source_content_type":"text/x-python","patch_set":12,"id":"ff570b3c_b8452746","line":1074,"range":{"start_line":1074,"start_character":12,"end_line":1074,"end_character":65},"in_reply_to":"ff570b3c_71ebb3db","updated":"2020-06-10 10:33:43.000000000","message":"ack... I\u0027m sure I got the idea of doing this from a bad example somewhere.\nDone.","commit_id":"4a017675b1645509cf5c437ef3fc6f2914b6030d"}]}