)]}'
{"neutron/agent/linux/openvswitch_firewall/firewall.py":[{"author":{"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]},"tag":"autogenerated:zuul:check","change_message_id":"26e6f1d5cc98f92a691c0c4329770eb597c2cc50","unresolved":false,"context_lines":[{"line_number":1400,"context_line":"                      {\u0027conj_id\u0027: conj_id,"},{"line_number":1401,"context_line":"                       \u0027sg_id\u0027: sec_group_id,"},{"line_number":1402,"context_line":"                       \u0027remote_sg_id\u0027: rule[\u0027remote_group_id\u0027],"},{"line_number":1403,"context_line":"                       \u0027remote_sg_id\u0027: rule[\u0027remote_address_group_id\u0027],"},{"line_number":1404,"context_line":"                       \u0027port_id\u0027: port.id})"},{"line_number":1405,"context_line":""},{"line_number":1406,"context_line":"            rule1 \u003d rule.copy()"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_3b3ded2b","line":1403,"updated":"2020-10-13 06:38:30.000000000","message":"pep8: F601 dictionary key \u0027remote_sg_id\u0027 repeated with different values","commit_id":"bac8dbbf09322e8e6f17b30c483e5f24258b5f5a"},{"author":{"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]},"tag":"autogenerated:zuul:check","change_message_id":"26e6f1d5cc98f92a691c0c4329770eb597c2cc50","unresolved":false,"context_lines":[{"line_number":1441,"context_line":"                    self._add_flow(**flow)"},{"line_number":1442,"context_line":""},{"line_number":1443,"context_line":"    def add_flows_from_rules(self, port):"},{"line_number":1444,"context_line":"        self._initialize_tracked_ingress(port)"},{"line_number":1445,"context_line":"        self._initialize_tracked_egress(port)"},{"line_number":1446,"context_line":"        LOG.debug(\u0027Creating flow rules for port %s that is port %d in OVS\u0027,"},{"line_number":1447,"context_line":"                  port.id, port.ofport)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_db32f15c","line":1444,"updated":"2020-10-13 06:38:30.000000000","message":"pep8: F601 dictionary key \u0027remote_sg_id\u0027 repeated with different values","commit_id":"bac8dbbf09322e8e6f17b30c483e5f24258b5f5a"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"29219099bc651158891fd1c6c59db591d64f25ea","unresolved":true,"context_lines":[{"line_number":226,"context_line":"        # Maps port_id to ofport number"},{"line_number":227,"context_line":"        self.unfiltered \u003d {}"},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"    def get_ag(self, ag_id):"},{"line_number":230,"context_line":"        return self.addr_groups.get(ag_id, None)"},{"line_number":231,"context_line":""},{"line_number":232,"context_line":"    def get_or_create_ag(self, ag_id):"}],"source_content_type":"text/x-python","patch_set":12,"id":"a6ba1272_42695b42","line":229,"range":{"start_line":229,"start_character":8,"end_line":229,"end_character":14},"updated":"2021-01-20 11:47:39.000000000","message":"nit: can You maybe use \"address_group\" instead of \"ag\" in the names of methods? IMHO it will be easier to read code in the future :)","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"e8e53909c4dde8d6ae437ce6a8d40e118a8fefe7","unresolved":true,"context_lines":[{"line_number":226,"context_line":"        # Maps port_id to ofport number"},{"line_number":227,"context_line":"        self.unfiltered \u003d {}"},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"    def get_ag(self, ag_id):"},{"line_number":230,"context_line":"        return self.addr_groups.get(ag_id, None)"},{"line_number":231,"context_line":""},{"line_number":232,"context_line":"    def get_or_create_ag(self, ag_id):"}],"source_content_type":"text/x-python","patch_set":12,"id":"05cc4c66_10ba3d72","line":229,"range":{"start_line":229,"start_character":8,"end_line":229,"end_character":14},"in_reply_to":"a6ba1272_42695b42","updated":"2021-01-20 22:02:47.000000000","message":"They were intended to aline with those methods have \"sg\" in name though, basically to indicate these methods provide similar functionality.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"29219099bc651158891fd1c6c59db591d64f25ea","unresolved":true,"context_lines":[{"line_number":229,"context_line":"    def get_ag(self, ag_id):"},{"line_number":230,"context_line":"        return self.addr_groups.get(ag_id, None)"},{"line_number":231,"context_line":""},{"line_number":232,"context_line":"    def get_or_create_ag(self, ag_id):"},{"line_number":233,"context_line":"        try:"},{"line_number":234,"context_line":"            addr_group \u003d self.addr_groups[ag_id]"},{"line_number":235,"context_line":"        except KeyError:"}],"source_content_type":"text/x-python","patch_set":12,"id":"2a266466_b9347afd","line":232,"updated":"2021-01-20 11:47:39.000000000","message":"same here, and in all other similar places","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"29219099bc651158891fd1c6c59db591d64f25ea","unresolved":true,"context_lines":[{"line_number":542,"context_line":"            if update:"},{"line_number":543,"context_line":"                self.update_flows_for_vlan(vlan_tag,"},{"line_number":544,"context_line":"                                           conj_id_to_remove\u003dconj_id_to_remove)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":""},{"line_number":547,"context_line":"class OVSFirewallDriver(firewall.FirewallDriver):"},{"line_number":548,"context_line":"    REQUIRED_PROTOCOLS \u003d ["}],"source_content_type":"text/x-python","patch_set":12,"id":"a3d55e03_7b06eb5a","line":545,"updated":"2021-01-20 11:47:39.000000000","message":"how this method is different from sg_removed above? For me both are the same.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"e8e53909c4dde8d6ae437ce6a8d40e118a8fefe7","unresolved":true,"context_lines":[{"line_number":542,"context_line":"            if update:"},{"line_number":543,"context_line":"                self.update_flows_for_vlan(vlan_tag,"},{"line_number":544,"context_line":"                                           conj_id_to_remove\u003dconj_id_to_remove)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":""},{"line_number":547,"context_line":"class OVSFirewallDriver(firewall.FirewallDriver):"},{"line_number":548,"context_line":"    REQUIRED_PROTOCOLS \u003d ["}],"source_content_type":"text/x-python","patch_set":12,"id":"f320b890_644acc0b","line":545,"in_reply_to":"a3d55e03_7b06eb5a","updated":"2021-01-20 22:02:47.000000000","message":"yup they are very similar but two differences are one calls delete_sg() and the other calls delete_ag(), also the sg_ag_tuple is (remote_sg_id, None) vs (None, remote_ag_id)","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"}],"neutron/agent/linux/openvswitch_firewall/rules.py":[{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"29219099bc651158891fd1c6c59db591d64f25ea","unresolved":true,"context_lines":[{"line_number":327,"context_line":"    flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[("},{"line_number":328,"context_line":"        ip_ver, direction)]] \u003d ip_prefix"},{"line_number":329,"context_line":""},{"line_number":330,"context_line":"    # TODO(hangyang) what if address group has an ANY ip without mac_addr"},{"line_number":331,"context_line":"    if any_src_ip:"},{"line_number":332,"context_line":"        flow_template[\u0027dl_src\u0027] \u003d mac_address"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"d129c2fe_442d988b","line":330,"updated":"2021-01-20 11:47:39.000000000","message":"I\u0027m not sure if I understand it. Address group is just list of IP addresses, how it can have or not have mac address?","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"5c52a646913286baf4384a594d8ade642c349749","unresolved":true,"context_lines":[{"line_number":327,"context_line":"    flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[("},{"line_number":328,"context_line":"        ip_ver, direction)]] \u003d ip_prefix"},{"line_number":329,"context_line":""},{"line_number":330,"context_line":"    # TODO(hangyang) what if address group has an ANY ip without mac_addr"},{"line_number":331,"context_line":"    if any_src_ip:"},{"line_number":332,"context_line":"        flow_template[\u0027dl_src\u0027] \u003d mac_address"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5e657a1a_d39bfad0","line":330,"in_reply_to":"1fdac16c_9c468b70","updated":"2021-02-05 00:05:31.000000000","message":"I tested it and found ovs returns error when setting dl_src to None for any_src_ip. So I choose to not generate flows for this case. Please see the latest patch for the change.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"36e318a1e93b61f84c8db718988ca65c8e05a65d","unresolved":true,"context_lines":[{"line_number":327,"context_line":"    flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[("},{"line_number":328,"context_line":"        ip_ver, direction)]] \u003d ip_prefix"},{"line_number":329,"context_line":""},{"line_number":330,"context_line":"    # TODO(hangyang) what if address group has an ANY ip without mac_addr"},{"line_number":331,"context_line":"    if any_src_ip:"},{"line_number":332,"context_line":"        flow_template[\u0027dl_src\u0027] \u003d mac_address"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9f6ebdff_ef5c1319","line":330,"in_reply_to":"2dd1fc9e_4bb25f10","updated":"2021-02-03 20:28:55.000000000","message":"For ANY ip address from address group, the mac address will also be None. I\u0027m not sure if that is ok for ovs flow.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"9b5cd98095a17289472ba2982e0eb4d8845ab2b9","unresolved":true,"context_lines":[{"line_number":327,"context_line":"    flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[("},{"line_number":328,"context_line":"        ip_ver, direction)]] \u003d ip_prefix"},{"line_number":329,"context_line":""},{"line_number":330,"context_line":"    # TODO(hangyang) what if address group has an ANY ip without mac_addr"},{"line_number":331,"context_line":"    if any_src_ip:"},{"line_number":332,"context_line":"        flow_template[\u0027dl_src\u0027] \u003d mac_address"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"2dd1fc9e_4bb25f10","line":330,"in_reply_to":"4b3a78e4_4bac8f62","updated":"2021-02-02 10:52:10.000000000","message":"I think that we should simply document the case when someone sets ANY ip address in address group. We shouldn\u0027t forbid it.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"2aaace5f8f18646058e214a9dd241fb008319fb4","unresolved":true,"context_lines":[{"line_number":327,"context_line":"    flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[("},{"line_number":328,"context_line":"        ip_ver, direction)]] \u003d ip_prefix"},{"line_number":329,"context_line":""},{"line_number":330,"context_line":"    # TODO(hangyang) what if address group has an ANY ip without mac_addr"},{"line_number":331,"context_line":"    if any_src_ip:"},{"line_number":332,"context_line":"        flow_template[\u0027dl_src\u0027] \u003d mac_address"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"7ccf0129_29372444","line":330,"in_reply_to":"5e657a1a_d39bfad0","updated":"2021-02-08 19:32:01.000000000","message":"I just found instead of setting dl_src\u003dNone which resulted in ovs error, we can just skip setting dl_src for any_src_ip without mac_address. And that worked in my testing. I have updated in ps #17.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"7b30655b9e2aeda4dbe78ba3afb360626512bc9e","unresolved":true,"context_lines":[{"line_number":327,"context_line":"    flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[("},{"line_number":328,"context_line":"        ip_ver, direction)]] \u003d ip_prefix"},{"line_number":329,"context_line":""},{"line_number":330,"context_line":"    # TODO(hangyang) what if address group has an ANY ip without mac_addr"},{"line_number":331,"context_line":"    if any_src_ip:"},{"line_number":332,"context_line":"        flow_template[\u0027dl_src\u0027] \u003d mac_address"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"1fdac16c_9c468b70","line":330,"in_reply_to":"84518435_6b72289d","updated":"2021-02-04 15:20:09.000000000","message":"yup, will do.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"34cbc06e875c95e8161866eb9875ce92efb8f349","unresolved":true,"context_lines":[{"line_number":327,"context_line":"    flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[("},{"line_number":328,"context_line":"        ip_ver, direction)]] \u003d ip_prefix"},{"line_number":329,"context_line":""},{"line_number":330,"context_line":"    # TODO(hangyang) what if address group has an ANY ip without mac_addr"},{"line_number":331,"context_line":"    if any_src_ip:"},{"line_number":332,"context_line":"        flow_template[\u0027dl_src\u0027] \u003d mac_address"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"84518435_6b72289d","line":330,"in_reply_to":"9f6ebdff_ef5c1319","updated":"2021-02-04 08:33:39.000000000","message":"I don\u0027t think it will be problem but can You test to be sure?","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"e8e53909c4dde8d6ae437ce6a8d40e118a8fefe7","unresolved":true,"context_lines":[{"line_number":327,"context_line":"    flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[("},{"line_number":328,"context_line":"        ip_ver, direction)]] \u003d ip_prefix"},{"line_number":329,"context_line":""},{"line_number":330,"context_line":"    # TODO(hangyang) what if address group has an ANY ip without mac_addr"},{"line_number":331,"context_line":"    if any_src_ip:"},{"line_number":332,"context_line":"        flow_template[\u0027dl_src\u0027] \u003d mac_address"},{"line_number":333,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"4b3a78e4_4bac8f62","line":330,"in_reply_to":"d129c2fe_442d988b","updated":"2021-01-20 22:02:47.000000000","message":"So my concern is that address group has no mac address, but can have any_src_ip(net.prefixlen \u003d\u003d 0). Based on the current code logic, it uses the mac_address instead in flow for any_src_ip. I\u0027d like to hear reviewer\u0027s suggestion about how should we handle it for address group. Should any_src_ip be disallowed in address group?","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"}],"neutron/db/securitygroups_rpc_base.py":[{"author":{"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]},"tag":"autogenerated:zuul:check","change_message_id":"b1b6ce68a0f9d28f0a7564a5a8fc483ed54b36b9","unresolved":false,"context_lines":[{"line_number":303,"context_line":""},{"line_number":304,"context_line":"                base_rule \u003d rule"},{"line_number":305,"context_line":"                if remote_group_id:"},{"line_number":306,"context_line":"                    port[\u0027security_group_source_groups\u0027].append(remote_group_id)"},{"line_number":307,"context_line":"                    ip_list \u003d [ip[0] for ip in ips[remote_group_id]]"},{"line_number":308,"context_line":"                else:"},{"line_number":309,"context_line":"                    port[\u0027security_group_remote_address_groups\u0027].append("}],"source_content_type":"text/x-python","patch_set":3,"id":"1f621f24_1e8d8431","line":306,"updated":"2020-11-13 02:13:05.000000000","message":"pep8: E501 line too long (80 \u003e 79 characters)","commit_id":"99180f72e2f5565c5f35d1fffe94cbd7c1e0cd25"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"29219099bc651158891fd1c6c59db591d64f25ea","unresolved":true,"context_lines":[{"line_number":238,"context_line":"                sg_info[\u0027security_groups\u0027][sg_id] \u003d []"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"        sg_info[\u0027sg_member_ips\u0027] \u003d remote_security_group_info"},{"line_number":241,"context_line":"        sg_info[\u0027remote_ag_ips\u0027] \u003d remote_address_group_info"},{"line_number":242,"context_line":"        # the provider rules do not belong to any security group, so these"},{"line_number":243,"context_line":"        # rules still reside in sg_info[\u0027devices\u0027] [port_id]"},{"line_number":244,"context_line":"        self._apply_provider_rule(context, sg_info[\u0027devices\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"84957ed7_787522eb","line":241,"updated":"2021-01-20 11:47:39.000000000","message":"why You can\u0027t simply add those additional IPs to sg_member_ips and proceed later with just one list/set of IP addresses? IMO for the backend they aren\u0027t really different in any way, am I right?","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"34cbc06e875c95e8161866eb9875ce92efb8f349","unresolved":true,"context_lines":[{"line_number":238,"context_line":"                sg_info[\u0027security_groups\u0027][sg_id] \u003d []"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"        sg_info[\u0027sg_member_ips\u0027] \u003d remote_security_group_info"},{"line_number":241,"context_line":"        sg_info[\u0027remote_ag_ips\u0027] \u003d remote_address_group_info"},{"line_number":242,"context_line":"        # the provider rules do not belong to any security group, so these"},{"line_number":243,"context_line":"        # rules still reside in sg_info[\u0027devices\u0027] [port_id]"},{"line_number":244,"context_line":"        self._apply_provider_rule(context, sg_info[\u0027devices\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"74a1505d_650bf258","line":241,"in_reply_to":"37f44a20_5e6d4260","updated":"2021-02-04 08:33:39.000000000","message":"But it is checking \"only\" member_ips. So in that case it would be scheduled to be cleaned if there would be not IPs comming from both sg and ag. Am I missing something?","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"7b30655b9e2aeda4dbe78ba3afb360626512bc9e","unresolved":true,"context_lines":[{"line_number":238,"context_line":"                sg_info[\u0027security_groups\u0027][sg_id] \u003d []"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"        sg_info[\u0027sg_member_ips\u0027] \u003d remote_security_group_info"},{"line_number":241,"context_line":"        sg_info[\u0027remote_ag_ips\u0027] \u003d remote_address_group_info"},{"line_number":242,"context_line":"        # the provider rules do not belong to any security group, so these"},{"line_number":243,"context_line":"        # rules still reside in sg_info[\u0027devices\u0027] [port_id]"},{"line_number":244,"context_line":"        self._apply_provider_rule(context, sg_info[\u0027devices\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"909457eb_fdfac4bf","line":241,"in_reply_to":"74a1505d_650bf258","updated":"2021-02-04 15:20:09.000000000","message":"def update_security_group_members(self, sg_id, member_ips):\n        self.sg_port_map.update_members(sg_id, member_ips)\n        if not member_ips:\n            self._schedule_sg_deletion_maybe(sg_id)\n    \n    def _schedule_sg_deletion_maybe(self, sg_id):\n        \"\"\"Schedule possible deletion of the given SG.\n\n        This function must be called when the number of ports\n        associated to sg_id drops to zero, as it isn\u0027t possible\n        to know SG deletions from agents due to RPC API design.\n        \"\"\"\n        sec_group \u003d self.sg_port_map.get_or_create_sg(sg_id)\n        if not sec_group.members or not sec_group.ports:\n            self.sg_to_delete.add(sg_id)\n\n    def _cleanup_stale_sg(self):\n        sg_to_delete \u003d self.sg_to_delete\n        self.sg_to_delete \u003d set()\n\n        for sg_id in sg_to_delete:\n            sec_group \u003d self.sg_port_map.get_sg(sg_id)\n            if sec_group.members and sec_group.ports:\n                # sec_group is still in use\n                continue\n\n            self.conj_ip_manager.sg_removed(sg_id)\n            self.sg_port_map.delete_sg(sg_id)\n\nYou are right, I was looking at the middle _schedule_sg_deletion_maybe() function but it only triggered by update_security_group_members() and port removal. For an address group, if its members ips drops to zero, the same functions can still remove it. I\u0027ll test locally to see if combining sg and ag works and will update the patch if so.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"e8e53909c4dde8d6ae437ce6a8d40e118a8fefe7","unresolved":true,"context_lines":[{"line_number":238,"context_line":"                sg_info[\u0027security_groups\u0027][sg_id] \u003d []"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"        sg_info[\u0027sg_member_ips\u0027] \u003d remote_security_group_info"},{"line_number":241,"context_line":"        sg_info[\u0027remote_ag_ips\u0027] \u003d remote_address_group_info"},{"line_number":242,"context_line":"        # the provider rules do not belong to any security group, so these"},{"line_number":243,"context_line":"        # rules still reside in sg_info[\u0027devices\u0027] [port_id]"},{"line_number":244,"context_line":"        self._apply_provider_rule(context, sg_info[\u0027devices\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"97cf32f5_d1b63423","line":241,"in_reply_to":"84957ed7_787522eb","updated":"2021-01-20 22:02:47.000000000","message":"The general reason is I don\u0027t want to mess up with the existing logic for handling remote security group IP changes. One case I can think of for the reason of using two set is when address group has duplicate IPs with sg_member_ips, if user delete the IP from address group, the same IP in sg_member_ips can be retained (which it should be). If sharing one set then that will result in an issue.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"5c52a646913286baf4384a594d8ade642c349749","unresolved":true,"context_lines":[{"line_number":238,"context_line":"                sg_info[\u0027security_groups\u0027][sg_id] \u003d []"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"        sg_info[\u0027sg_member_ips\u0027] \u003d remote_security_group_info"},{"line_number":241,"context_line":"        sg_info[\u0027remote_ag_ips\u0027] \u003d remote_address_group_info"},{"line_number":242,"context_line":"        # the provider rules do not belong to any security group, so these"},{"line_number":243,"context_line":"        # rules still reside in sg_info[\u0027devices\u0027] [port_id]"},{"line_number":244,"context_line":"        self._apply_provider_rule(context, sg_info[\u0027devices\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"463952ef_d6435bcc","line":241,"in_reply_to":"909457eb_fdfac4bf","updated":"2021-02-05 00:05:31.000000000","message":"Please see the latest patch for the change, I have combined sg and ag in firewall.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"9b5cd98095a17289472ba2982e0eb4d8845ab2b9","unresolved":true,"context_lines":[{"line_number":238,"context_line":"                sg_info[\u0027security_groups\u0027][sg_id] \u003d []"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"        sg_info[\u0027sg_member_ips\u0027] \u003d remote_security_group_info"},{"line_number":241,"context_line":"        sg_info[\u0027remote_ag_ips\u0027] \u003d remote_address_group_info"},{"line_number":242,"context_line":"        # the provider rules do not belong to any security group, so these"},{"line_number":243,"context_line":"        # rules still reside in sg_info[\u0027devices\u0027] [port_id]"},{"line_number":244,"context_line":"        self._apply_provider_rule(context, sg_info[\u0027devices\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"b3db0183_a77d0ae5","line":241,"in_reply_to":"97cf32f5_d1b63423","updated":"2021-02-02 10:52:10.000000000","message":"IMHO that could be resolved by doing some db query to check if such IP address which is removed from the AG isn\u0027t also used by some sg_member. I think that code is already in place so we would just need to reuse it.\nAnd that could make backend implementation much smaller probably.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"36e318a1e93b61f84c8db718988ca65c8e05a65d","unresolved":true,"context_lines":[{"line_number":238,"context_line":"                sg_info[\u0027security_groups\u0027][sg_id] \u003d []"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"        sg_info[\u0027sg_member_ips\u0027] \u003d remote_security_group_info"},{"line_number":241,"context_line":"        sg_info[\u0027remote_ag_ips\u0027] \u003d remote_address_group_info"},{"line_number":242,"context_line":"        # the provider rules do not belong to any security group, so these"},{"line_number":243,"context_line":"        # rules still reside in sg_info[\u0027devices\u0027] [port_id]"},{"line_number":244,"context_line":"        self._apply_provider_rule(context, sg_info[\u0027devices\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"f7bfd280_94754818","line":241,"in_reply_to":"b3db0183_a77d0ae5","updated":"2021-02-03 20:28:55.000000000","message":"sg_info[\u0027sg_member_ips\u0027] \u003d {\u0027sg_id\u0027: {IPs}} if we merge the remote_ag_ips into it, then later when agent calls firewall to update addresses, it won\u0027t be able to know either create a local security group object or address group object as the ids in sg_member_ips can be either sg_id or ag_id. See: https://review.opendev.org/c/openstack/neutron/+/757650/12/neutron/agent/securitygroups_rpc.py#218 and https://review.opendev.org/c/openstack/neutron/+/757650/12/neutron/agent/linux/openvswitch_firewall/firewall.py#841. Or do you think we should also combine SG and AG objects in firewall?","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"},{"author":{"_account_id":28159,"name":"Hang Yang","email":"hangyang@yahooinc.com","username":"hangyang"},"change_message_id":"4fb0d48c15e604fdd0b104462b6e5bca59baa743","unresolved":true,"context_lines":[{"line_number":238,"context_line":"                sg_info[\u0027security_groups\u0027][sg_id] \u003d []"},{"line_number":239,"context_line":""},{"line_number":240,"context_line":"        sg_info[\u0027sg_member_ips\u0027] \u003d remote_security_group_info"},{"line_number":241,"context_line":"        sg_info[\u0027remote_ag_ips\u0027] \u003d remote_address_group_info"},{"line_number":242,"context_line":"        # the provider rules do not belong to any security group, so these"},{"line_number":243,"context_line":"        # rules still reside in sg_info[\u0027devices\u0027] [port_id]"},{"line_number":244,"context_line":"        self._apply_provider_rule(context, sg_info[\u0027devices\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"37f44a20_5e6d4260","line":241,"in_reply_to":"f7bfd280_94754818","updated":"2021-02-03 22:10:23.000000000","message":"I thought over reusing the SG object for AG in firewall but at least one obstacle I see is for the cleanup flow https://review.opendev.org/c/openstack/neutron/+/757650/12/neutron/agent/linux/openvswitch_firewall/firewall.py#851 when a sg has no members or no ports associated with then we trigger the cleanup. But for address group, it never has any port association and we do cleanup only when it has no addresses. So currently I still think we need differentiate sg and ag in firewall thus also needed in sg_info data.","commit_id":"6c53af9159dff067b8754f063e17ec5099d5b807"}]}