)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"change_message_id":"04b63857f959954b26ecc48cce1a7a4615a0b565","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"5e01e896_4b7f2576","updated":"2022-05-10 10:03:42.000000000","message":"Thanks a lot Elvira!\nI have some concerns about this patch.\n\nFirst, this adds two ACLs per Security Group which is what the neutron_pg_drop was aiming to avoid.\n\n- If we take approach (just for logging purposes), I think that this should be a series of patches that will as well remove the neutron_pg_drop. Otherwise, ovn-controller will end up expanding 2 drop rules *per port*. (**note below)\n\n- Also, please note that we\u0027re trying to remove the neutron_pg_drop (in favor of neutron_pg_allow) because, at scale, this results in a large amount of flows that we can avoid by changing the default firewall policy at core OVN level. This patch of yours would probably defeat this purpose.\nhttps://review.opendev.org/c/openstack/neutron/+/839066 \n\n- If a performance hit is \u0027allowed\u0027 for logging purposes, we can consider having these ACLs only when SG logging is enabled. This is a bit complicated as we\u0027ll need to add/delete them upon configuration changes (hopefully not often ;) \n\n\n** We should actually verify that ovn-controller will expand them twice. I\u0027m just guessing. \n","commit_id":"29d2252b444e254cad183d8aa2420425eab92a48"}],"neutron/common/ovn/acl.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"c5a536d6ce3bf0dedad68a41b6df3ea9447163bb","unresolved":true,"context_lines":[{"line_number":153,"context_line":"    return acl_list"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"def add_drop_acl_for_security_group(ovn, pg_name, txn):"},{"line_number":157,"context_line":"    acl_list \u003d []"},{"line_number":158,"context_line":"    for direction, p in ((\u0027from-lport\u0027, \u0027inport\u0027),"},{"line_number":159,"context_line":"                         (\u0027to-lport\u0027, \u0027outport\u0027)):"}],"source_content_type":"text/x-python","patch_set":6,"id":"838544ca_3660e731","line":156,"range":{"start_line":156,"start_character":4,"end_line":156,"end_character":35},"updated":"2022-07-12 10:05:53.000000000","message":"sorry, I\u0027m being pedantic here. The previous method is called \"add_acls_for_drop_xxx\". Why don\u0027t we call this method \"add_acls_for_drop_security_group()\" (same prefix)?","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"4b1788458c906fde8c3b18bec82664c8db130e57","unresolved":true,"context_lines":[{"line_number":153,"context_line":"    return acl_list"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"def add_drop_acl_for_security_group(ovn, pg_name, txn):"},{"line_number":157,"context_line":"    acl_list \u003d []"},{"line_number":158,"context_line":"    for direction, p in ((\u0027from-lport\u0027, \u0027inport\u0027),"},{"line_number":159,"context_line":"                         (\u0027to-lport\u0027, \u0027outport\u0027)):"}],"source_content_type":"text/x-python","patch_set":6,"id":"ff5d82b3_f2174350","line":156,"range":{"start_line":156,"start_character":4,"end_line":156,"end_character":35},"in_reply_to":"838544ca_3660e731","updated":"2022-07-26 10:21:31.000000000","message":"It might be more beautiful but it wouldn\u0027t be a good explanation as there is no such thing as drop security groups, this function purpose is to create drop acls that are added to port groups associated to security groups.","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"c5a536d6ce3bf0dedad68a41b6df3ea9447163bb","unresolved":true,"context_lines":[{"line_number":166,"context_line":"               \"direction\": direction,"},{"line_number":167,"context_line":"               \"match\": \u0027%s \u003d\u003d @%s \u0026\u0026 ip\u0027 % (p, pg_name)}"},{"line_number":168,"context_line":"        acl_list.append(acl)"},{"line_number":169,"context_line":"        txn.add(ovn.pg_acl_add(**acl, may_exist\u003dTrue))"},{"line_number":170,"context_line":"    return acl_list"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"1386aa15_2f1a2d1d","line":169,"range":{"start_line":169,"start_character":16,"end_line":169,"end_character":19},"updated":"2022-07-12 10:05:53.000000000","message":"I know we don\u0027t have a standard for naming these IDLs, but \"ovn_nb\" or \"nb_idl \"could be better in this case.","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"4b1788458c906fde8c3b18bec82664c8db130e57","unresolved":false,"context_lines":[{"line_number":166,"context_line":"               \"direction\": direction,"},{"line_number":167,"context_line":"               \"match\": \u0027%s \u003d\u003d @%s \u0026\u0026 ip\u0027 % (p, pg_name)}"},{"line_number":168,"context_line":"        acl_list.append(acl)"},{"line_number":169,"context_line":"        txn.add(ovn.pg_acl_add(**acl, may_exist\u003dTrue))"},{"line_number":170,"context_line":"    return acl_list"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"545e177f_37342158","line":169,"range":{"start_line":169,"start_character":16,"end_line":169,"end_character":19},"in_reply_to":"1386aa15_2f1a2d1d","updated":"2022-07-26 10:21:31.000000000","message":"Done","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"}],"neutron/common/ovn/constants.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"c5a536d6ce3bf0dedad68a41b6df3ea9447163bb","unresolved":true,"context_lines":[{"line_number":78,"context_line":""},{"line_number":79,"context_line":"# OVN ACLs have priorities.  The highest priority ACL that matches is the one"},{"line_number":80,"context_line":"# that takes effect.  Our choice of priority numbers is arbitrary, but it"},{"line_number":81,"context_line":"# leaves room above and below the ACLs we create.  We only need two priorities."},{"line_number":82,"context_line":"# The first is for all the things we allow.  The second is for dropping traffic"},{"line_number":83,"context_line":"# by default."},{"line_number":84,"context_line":"ACL_PRIORITY_ALLOW \u003d 1003"},{"line_number":85,"context_line":"ACL_PRIORITY_DROP_SG \u003d 1002"},{"line_number":86,"context_line":"ACL_PRIORITY_DROP \u003d 1001"}],"source_content_type":"text/x-python","patch_set":6,"id":"02e4daed_6e0bbfcb","line":83,"range":{"start_line":81,"start_character":51,"end_line":83,"end_character":13},"updated":"2022-07-12 10:05:53.000000000","message":"We need to change this.","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"c5a536d6ce3bf0dedad68a41b6df3ea9447163bb","unresolved":true,"context_lines":[{"line_number":81,"context_line":"# leaves room above and below the ACLs we create.  We only need two priorities."},{"line_number":82,"context_line":"# The first is for all the things we allow.  The second is for dropping traffic"},{"line_number":83,"context_line":"# by default."},{"line_number":84,"context_line":"ACL_PRIORITY_ALLOW \u003d 1003"},{"line_number":85,"context_line":"ACL_PRIORITY_DROP_SG \u003d 1002"},{"line_number":86,"context_line":"ACL_PRIORITY_DROP \u003d 1001"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"ACL_ACTION_DROP \u003d \u0027drop\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"f98cfbe8_8f2c62b5","line":85,"range":{"start_line":84,"start_character":0,"end_line":85,"end_character":27},"updated":"2022-07-12 10:05:53.000000000","message":"Hold on, that will affect existing deployments. Any ACL created before updating, will have the previous values. If that\u0027s the case (please, check it), you need a migration tool here.","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"4b1788458c906fde8c3b18bec82664c8db130e57","unresolved":true,"context_lines":[{"line_number":81,"context_line":"# leaves room above and below the ACLs we create.  We only need two priorities."},{"line_number":82,"context_line":"# The first is for all the things we allow.  The second is for dropping traffic"},{"line_number":83,"context_line":"# by default."},{"line_number":84,"context_line":"ACL_PRIORITY_ALLOW \u003d 1003"},{"line_number":85,"context_line":"ACL_PRIORITY_DROP_SG \u003d 1002"},{"line_number":86,"context_line":"ACL_PRIORITY_DROP \u003d 1001"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"ACL_ACTION_DROP \u003d \u0027drop\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"973d2dd5_f422baa0","line":85,"range":{"start_line":84,"start_character":0,"end_line":85,"end_character":27},"in_reply_to":"f98cfbe8_8f2c62b5","updated":"2022-07-26 10:21:31.000000000","message":"I\u0027m working on this","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"c5a536d6ce3bf0dedad68a41b6df3ea9447163bb","unresolved":true,"context_lines":[{"line_number":2247,"context_line":"        if self._nb_idl.get_port_group(pg_name):"},{"line_number":2248,"context_line":"            txn.add(self._nb_idl.pg_del_ports(pg_name, port))"},{"line_number":2249,"context_line":""},{"line_number":2250,"context_line":"    def delete_security_group(self, context, security_group_id):"},{"line_number":2251,"context_line":"        with self._nb_idl.transaction(check_error\u003dTrue) as txn:"},{"line_number":2252,"context_line":"            name \u003d utils.ovn_port_group_name(security_group_id)"},{"line_number":2253,"context_line":"            txn.add(self._nb_idl.pg_del(name\u003dname, if_exists\u003dTrue))"}],"source_content_type":"text/x-python","patch_set":6,"id":"6f81bd6f_0a599910","line":2250,"range":{"start_line":2250,"start_character":8,"end_line":2250,"end_character":29},"updated":"2022-07-12 10:05:53.000000000","message":"The drop ACL depends on the SG, not the rules (the ACLs per rule are deleted in \"_process_security_group_rule\"). Shouldn\u0027t you delete this drop ACL group here?","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"4b1788458c906fde8c3b18bec82664c8db130e57","unresolved":true,"context_lines":[{"line_number":2247,"context_line":"        if self._nb_idl.get_port_group(pg_name):"},{"line_number":2248,"context_line":"            txn.add(self._nb_idl.pg_del_ports(pg_name, port))"},{"line_number":2249,"context_line":""},{"line_number":2250,"context_line":"    def delete_security_group(self, context, security_group_id):"},{"line_number":2251,"context_line":"        with self._nb_idl.transaction(check_error\u003dTrue) as txn:"},{"line_number":2252,"context_line":"            name \u003d utils.ovn_port_group_name(security_group_id)"},{"line_number":2253,"context_line":"            txn.add(self._nb_idl.pg_del(name\u003dname, if_exists\u003dTrue))"}],"source_content_type":"text/x-python","patch_set":6,"id":"97eb360b_ad985ea9","line":2250,"range":{"start_line":2250,"start_character":8,"end_line":2250,"end_character":29},"in_reply_to":"6f81bd6f_0a599910","updated":"2022-07-26 10:21:31.000000000","message":"They get deleted, because I don\u0027t create a new port group, I just add delete ACLs to the SG port group that already existed. Therefore, the pg_del in line 2286 should be enough","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"}],"neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"c5a536d6ce3bf0dedad68a41b6df3ea9447163bb","unresolved":true,"context_lines":[{"line_number":254,"context_line":"        # if allow-stateless supported, we have to fetch groups to determine if"},{"line_number":255,"context_line":"        # stateful is set"},{"line_number":256,"context_line":"        if self._ovn_client.is_allow_stateless_supported():"},{"line_number":257,"context_line":"            for sg in self.core_plugin.get_security_groups(ctx):"},{"line_number":258,"context_line":"                stateful \u003d sg.get(\"stateful\", True)"},{"line_number":259,"context_line":"                pg_name \u003d utils.ovn_port_group_name(sg[\u0027id\u0027])"},{"line_number":260,"context_line":"                for sgr in self.core_plugin.get_security_group_rules("}],"source_content_type":"text/x-python","patch_set":6,"id":"2514fc4e_44440603","line":257,"range":{"start_line":257,"start_character":39,"end_line":257,"end_character":58},"updated":"2022-07-12 10:05:53.000000000","message":"Let\u0027s save DB accesses: please, cache this query at the beginning of \"sync_acls\" instead of calling it twice here and in L275.","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"4b1788458c906fde8c3b18bec82664c8db130e57","unresolved":false,"context_lines":[{"line_number":254,"context_line":"        # if allow-stateless supported, we have to fetch groups to determine if"},{"line_number":255,"context_line":"        # stateful is set"},{"line_number":256,"context_line":"        if self._ovn_client.is_allow_stateless_supported():"},{"line_number":257,"context_line":"            for sg in self.core_plugin.get_security_groups(ctx):"},{"line_number":258,"context_line":"                stateful \u003d sg.get(\"stateful\", True)"},{"line_number":259,"context_line":"                pg_name \u003d utils.ovn_port_group_name(sg[\u0027id\u0027])"},{"line_number":260,"context_line":"                for sgr in self.core_plugin.get_security_group_rules("}],"source_content_type":"text/x-python","patch_set":6,"id":"7a147164_3c153f24","line":257,"range":{"start_line":257,"start_character":39,"end_line":257,"end_character":58},"in_reply_to":"2514fc4e_44440603","updated":"2022-07-26 10:21:31.000000000","message":"Done","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"},{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"c5a536d6ce3bf0dedad68a41b6df3ea9447163bb","unresolved":true,"context_lines":[{"line_number":273,"context_line":"            ovn_const.OVN_DROP_PORT_GROUP_NAME)"},{"line_number":274,"context_line":"        # Add drop acls per security group"},{"line_number":275,"context_line":"        for sg in self.core_plugin.get_security_groups(ctx):"},{"line_number":276,"context_line":"            drop_acls \u003d acl_utils.add_acls_for_drop_port_group("},{"line_number":277,"context_line":"                utils.ovn_port_group_name(sg[\u0027id\u0027]))"},{"line_number":278,"context_line":"            for acl in drop_acls:"},{"line_number":279,"context_line":"                acl[\"priority\"] \u003d ovn_const.ACL_PRIORITY_DROP_SG"}],"source_content_type":"text/x-python","patch_set":6,"id":"d244f4dd_eccc207f","line":276,"range":{"start_line":276,"start_character":34,"end_line":276,"end_character":62},"updated":"2022-07-12 10:05:53.000000000","message":"Why for port group? Shouldn\u0027t this be for security group (\"add_drop_acl_for_security_group\")?","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"},{"author":{"_account_id":32586,"name":"Elvira García Ruiz","display_name":"Elvira","email":"egarciar@redhat.com","username":"elvira"},"change_message_id":"4b1788458c906fde8c3b18bec82664c8db130e57","unresolved":false,"context_lines":[{"line_number":273,"context_line":"            ovn_const.OVN_DROP_PORT_GROUP_NAME)"},{"line_number":274,"context_line":"        # Add drop acls per security group"},{"line_number":275,"context_line":"        for sg in self.core_plugin.get_security_groups(ctx):"},{"line_number":276,"context_line":"            drop_acls \u003d acl_utils.add_acls_for_drop_port_group("},{"line_number":277,"context_line":"                utils.ovn_port_group_name(sg[\u0027id\u0027]))"},{"line_number":278,"context_line":"            for acl in drop_acls:"},{"line_number":279,"context_line":"                acl[\"priority\"] \u003d ovn_const.ACL_PRIORITY_DROP_SG"}],"source_content_type":"text/x-python","patch_set":6,"id":"ef514276_363455c3","line":276,"range":{"start_line":276,"start_character":34,"end_line":276,"end_character":62},"in_reply_to":"d244f4dd_eccc207f","updated":"2022-07-26 10:21:31.000000000","message":"Done","commit_id":"71e9db7373fb33d80f5bacf7b97f39a6ae12a1f8"}]}