From 3e316cbf24516e4ce3fc278e465e19b87f7f71ff Mon Sep 17 00:00:00 2001 From: Saikrishna Arcot Date: Thu, 23 Feb 2023 14:49:51 -0800 Subject: [PATCH] Don't create the members@ array in config_db for PC when reading from minigraph (#13660) Fixes #11873. #### Why I did it When loading from minigraph, for port channels, don't create the members@ array in config_db in the PORTCHANNEL table. This is no longer needed or used. In addition, when adding a port channel member from the CLI, that member doesn't get added into the members@ array, resulting in a bit of inconsistency. This gets rid of that inconsistency. --- src/sonic-config-engine/minigraph.py | 21 +++++++++++-------- src/sonic-config-engine/tests/test_cfggen.py | 4 ++-- .../tests/test_minigraph_case.py | 2 +- .../tests/test_multinpu_cfggen.py | 10 ++++----- src/sonic-yang-mgmt/tests/test_cfghelp.py | 20 +++++++++--------- .../tests/files/sample_config_db.json | 13 ------------ .../yang_model_tests/tests_config/mclag.json | 9 -------- .../tests_config/portchannel.json | 18 ---------------- .../tests_config/vlan_sub_interface.json | 12 ----------- .../yang-models/sonic-portchannel.yang | 17 --------------- 10 files changed, 30 insertions(+), 96 deletions(-) diff --git a/src/sonic-config-engine/minigraph.py b/src/sonic-config-engine/minigraph.py index 094d62bf2674..313c2ebfaded 100644 --- a/src/sonic-config-engine/minigraph.py +++ b/src/sonic-config-engine/minigraph.py @@ -532,9 +532,9 @@ def parse_dpg(dpg, hname): intfs_inpc.append(pcmbr_list[i]) pc_members[(pcintfname, pcmbr_list[i])] = {} if pcintf.find(str(QName(ns, "Fallback"))) != None: - pcs[pcintfname] = {'members': pcmbr_list, 'fallback': pcintf.find(str(QName(ns, "Fallback"))).text, 'min_links': str(int(math.ceil(len() * 0.75))), 'lacp_key': 'auto'} + pcs[pcintfname] = {'fallback': pcintf.find(str(QName(ns, "Fallback"))).text, 'min_links': str(int(math.ceil(len() * 0.75))), 'lacp_key': 'auto'} else: - pcs[pcintfname] = {'members': pcmbr_list, 'min_links': str(int(math.ceil(len(pcmbr_list) * 0.75))), 'lacp_key': 'auto' } + pcs[pcintfname] = {'min_links': str(int(math.ceil(len(pcmbr_list) * 0.75))), 'lacp_key': 'auto' } port_nhipv4_map = {} port_nhipv6_map = {} nhg_int = "" @@ -1244,7 +1244,7 @@ def filter_acl_table_for_backend(acls, vlan_members): } return filter_acls -def filter_acl_table_bindings(acls, neighbors, port_channels, sub_role, device_type, is_storage_device, vlan_members): +def filter_acl_table_bindings(acls, neighbors, port_channels, pc_members, sub_role, device_type, is_storage_device, vlan_members): if device_type == 'BackEndToRRouter' and is_storage_device: return filter_acl_table_for_backend(acls, vlan_members) @@ -1263,8 +1263,8 @@ def filter_acl_table_bindings(acls, neighbors, port_channels, sub_role, device_t # Get the front panel port channel. for port_channel_intf in port_channels: - backend_port_channel = any(lag_member in backplane_port_list \ - for lag_member in port_channels[port_channel_intf]['members']) + backend_port_channel = any(lag_member[1] in backplane_port_list \ + for lag_member in list(pc_members.keys()) if lag_member[0] == port_channel_intf) if not backend_port_channel: front_port_channel_intf.append(port_channel_intf) @@ -1763,12 +1763,15 @@ def parse_xml(filename, platform=None, port_config_file=None, asic_name=None, hw if port_config_file: port_set = set(ports.keys()) - for (pc_name, mbr_map) in list(pcs.items()): + for (pc_name, pc_member) in list(pc_members.keys()): # remove portchannels that contain ports not existing in port_config.ini # when port_config.ini exists - if not set(mbr_map['members']).issubset(port_set): - print("Warning: ignore '%s' as part of its member interfaces is not in the port_config.ini" % pc_name, file=sys.stderr) + if (pc_name, pc_member) in pc_members and pc_member not in port_set: + print("Warning: ignore '%s' as at least one of its member interfaces ('%s') is not in the port_config.ini" % (pc_name, pc_member), file=sys.stderr) del pcs[pc_name] + pc_mbr_del_keys = [f for f in list(pc_members.keys()) if f[0] == pc_name] + for pc_mbr_del_key in pc_mbr_del_keys: + del pc_members[pc_mbr_del_key] # set default port channel MTU as 9100 and admin status up and default TPID 0x8100 for pc in pcs.values(): @@ -1872,7 +1875,7 @@ def parse_xml(filename, platform=None, port_config_file=None, asic_name=None, hw results['DHCP_RELAY'] = dhcp_relay_table results['NTP_SERVER'] = dict((item, {}) for item in ntp_servers) results['TACPLUS_SERVER'] = dict((item, {'priority': '1', 'tcp_port': '49'}) for item in tacacs_servers) - results['ACL_TABLE'] = filter_acl_table_bindings(acls, neighbors, pcs, sub_role, current_device['type'], is_storage_device, vlan_members) + results['ACL_TABLE'] = filter_acl_table_bindings(acls, neighbors, pcs, pc_members, sub_role, current_device['type'], is_storage_device, vlan_members) results['FEATURE'] = { 'telemetry': { 'state': 'enabled' diff --git a/src/sonic-config-engine/tests/test_cfggen.py b/src/sonic-config-engine/tests/test_cfggen.py index 9b1d0b721983..3ab69fbf6cc8 100644 --- a/src/sonic-config-engine/tests/test_cfggen.py +++ b/src/sonic-config-engine/tests/test_cfggen.py @@ -328,7 +328,7 @@ def test_minigraph_portchannels(self, **kwargs): output = self.run_script(argument) self.assertEqual( utils.to_dict(output.strip()), - utils.to_dict("{'PortChannel1': {'admin_status': 'up', 'min_links': '1', 'members': ['Ethernet4'], 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}}") + utils.to_dict("{'PortChannel1': {'admin_status': 'up', 'min_links': '1', 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}}") ) def test_minigraph_portchannel_with_more_member(self): @@ -336,7 +336,7 @@ def test_minigraph_portchannel_with_more_member(self): output = self.run_script(argument) self.assertEqual( utils.to_dict(output.strip()), - utils.to_dict("{'PortChannel01': {'admin_status': 'up', 'min_links': '3', 'members': ['Ethernet112', 'Ethernet116', 'Ethernet120', 'Ethernet124'], 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}}")) + utils.to_dict("{'PortChannel01': {'admin_status': 'up', 'min_links': '3', 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}}")) def test_minigraph_portchannel_members(self): argument = ['-m', self.sample_graph_pc_test, '-p', self.port_config, '-v', "PORTCHANNEL_MEMBER.keys()|list"] diff --git a/src/sonic-config-engine/tests/test_minigraph_case.py b/src/sonic-config-engine/tests/test_minigraph_case.py index 86bd92ebe362..7f0319fa3413 100644 --- a/src/sonic-config-engine/tests/test_minigraph_case.py +++ b/src/sonic-config-engine/tests/test_minigraph_case.py @@ -158,7 +158,7 @@ def test_minigraph_portchannels(self): output = self.run_script(argument) self.assertEqual( utils.to_dict(output.strip()), - utils.to_dict("{'PortChannel01': {'admin_status': 'up', 'min_links': '1', 'members': ['Ethernet4'], 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}}") + utils.to_dict("{'PortChannel01': {'admin_status': 'up', 'min_links': '1', 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}}") ) def test_minigraph_console_mgmt_feature(self): diff --git a/src/sonic-config-engine/tests/test_multinpu_cfggen.py b/src/sonic-config-engine/tests/test_multinpu_cfggen.py index be1f677e178e..6eb2c8674abd 100644 --- a/src/sonic-config-engine/tests/test_multinpu_cfggen.py +++ b/src/sonic-config-engine/tests/test_multinpu_cfggen.py @@ -161,16 +161,16 @@ def test_frontend_asic_portchannels(self): argument = ["-m", self.sample_graph, "-p", self.port_config[0], "-n", "asic0", "--var-json", "PORTCHANNEL"] output = json.loads(self.run_script(argument)) self.assertDictEqual(output, \ - {'PortChannel0002': {'admin_status': 'up', 'min_links': '2', 'members': ['Ethernet0', 'Ethernet4'], 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}, - 'PortChannel4001': {'admin_status': 'up', 'min_links': '2', 'members': ['Ethernet-BP0', 'Ethernet-BP4'], 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}, - 'PortChannel4002': {'admin_status': 'up', 'min_links': '2', 'members': ['Ethernet-BP8', 'Ethernet-BP12'], 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}}) + {'PortChannel0002': {'admin_status': 'up', 'min_links': '2', 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}, + 'PortChannel4001': {'admin_status': 'up', 'min_links': '2', 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}, + 'PortChannel4002': {'admin_status': 'up', 'min_links': '2', 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}}) def test_backend_asic_portchannels(self): argument = ["-m", self.sample_graph, "-p", self.port_config[3], "-n", "asic3", "--var-json", "PORTCHANNEL"] output = json.loads(self.run_script(argument)) self.assertDictEqual(output, \ - {'PortChannel4013': {'admin_status': 'up', 'min_links': '2', 'members': ['Ethernet-BP384', 'Ethernet-BP388'], 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}, - 'PortChannel4014': {'admin_status': 'up', 'min_links': '2', 'members': ['Ethernet-BP392', 'Ethernet-BP396'], 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}}) + {'PortChannel4013': {'admin_status': 'up', 'min_links': '2', 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}, + 'PortChannel4014': {'admin_status': 'up', 'min_links': '2', 'mtu': '9100', 'tpid': '0x8100', 'lacp_key': 'auto'}}) def test_frontend_asic_portchannel_mem(self): argument = ["-m", self.sample_graph, "-p", self.port_config[0], "-n", "asic0", "-v", "PORTCHANNEL_MEMBER.keys()|list"] diff --git a/src/sonic-yang-mgmt/tests/test_cfghelp.py b/src/sonic-yang-mgmt/tests/test_cfghelp.py index 5867c78e5282..2eebebe7be3b 100644 --- a/src/sonic-yang-mgmt/tests/test_cfghelp.py +++ b/src/sonic-yang-mgmt/tests/test_cfghelp.py @@ -71,17 +71,17 @@ """ -portchannel_table_field_output="""\ +vlan_table_field_output="""\ -PORTCHANNEL -Description: PORTCHANNEL part of config_db.json +VLAN +Description: VLAN part of config_db.json key - name -+---------+-------------------------------------------+-------------+-----------+-------------+ -| Field | Description | Mandatory | Default | Reference | -+=========+===========================================+=============+===========+=============+ -| members | The field contains list of unique members | | | PORT:name | -+---------+-------------------------------------------+-------------+-----------+-------------+ ++--------------+------------------------------------------------------------------------+-------------+-----------+-------------+ +| Field | Description | Mandatory | Default | Reference | ++==============+========================================================================+=============+===========+=============+ +| dhcp_servers | The field contains list of unique membersConfigure the dhcp v4 servers | | | | ++--------------+------------------------------------------------------------------------+-------------+-----------+-------------+ """ @@ -153,9 +153,9 @@ def test_single_field(self): self.assertEqual(output, techsupport_table_field_output) def test_leaf_list(self): - argument = ['-t', 'PORTCHANNEL', '-f', 'members'] + argument = ['-t', 'VLAN', '-f', 'dhcp_servers'] output = self.run_script(argument) - self.assertEqual(output, portchannel_table_field_output) + self.assertEqual(output, vlan_table_field_output) def test_leaf_list_map(self): argument = ['-t', 'DSCP_TO_TC_MAP'] diff --git a/src/sonic-yang-models/tests/files/sample_config_db.json b/src/sonic-yang-models/tests/files/sample_config_db.json index 375f7d24e4b5..8b80631f4fb4 100644 --- a/src/sonic-yang-models/tests/files/sample_config_db.json +++ b/src/sonic-yang-models/tests/files/sample_config_db.json @@ -86,9 +86,6 @@ "PortChannel0003": { "admin_status": "up", "min_links": "1", - "members": [ - "Ethernet1" - ], "tpid": "0x8100", "mtu": "9100", "lacp_key": "auto" @@ -96,9 +93,6 @@ "PortChannel0004": { "admin_status": "up", "min_links": "1", - "members": [ - "Ethernet2" - ], "tpid": "0x9200", "mtu": "9100", "lacp_key": "auto" @@ -106,19 +100,12 @@ "PortChannel2": { "admin_status": "up", "min_links": "1", - "members": [ - "Ethernet12" - ], "tpid": "0x9200", "mtu": "9100", "lacp_key": "auto" }, "PortChannel42": { "admin_status": "up", - "members": [ - "Ethernet-BP0", - "Ethernet-BP4" - ], "min_links": "2", "mtu": "9100", "tpid": "0x8100" diff --git a/src/sonic-yang-models/tests/yang_model_tests/tests_config/mclag.json b/src/sonic-yang-models/tests/yang_model_tests/tests_config/mclag.json index fdb2661a973a..3d2ca3c92be6 100644 --- a/src/sonic-yang-models/tests/yang_model_tests/tests_config/mclag.json +++ b/src/sonic-yang-models/tests/yang_model_tests/tests_config/mclag.json @@ -29,9 +29,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1", "mtu": "9100", "lacp_key": "auto", @@ -39,9 +36,6 @@ }, { "admin_status": "up", - "members": [ - "Ethernet10" - ], "min_links": "1", "mtu": "9100", "lacp_key": "auto", @@ -151,9 +145,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1", "mtu": "9100", "lacp_key": "auto", diff --git a/src/sonic-yang-models/tests/yang_model_tests/tests_config/portchannel.json b/src/sonic-yang-models/tests/yang_model_tests/tests_config/portchannel.json index 6c26e9ce83e6..ba0762e5ebc6 100644 --- a/src/sonic-yang-models/tests/yang_model_tests/tests_config/portchannel.json +++ b/src/sonic-yang-models/tests/yang_model_tests/tests_config/portchannel.json @@ -21,9 +21,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1", "mtu": "9100", "tpid": "0x8100", @@ -55,9 +52,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1024", "mtu": "9100", "name": "PortChannel0001" @@ -87,9 +81,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1025", "mtu": "9100", "name": "PortChannel0001" @@ -303,9 +294,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1", "mtu": "9100", "name": "PortChannel0001" @@ -350,9 +338,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1", "mtu": "9100", "name": "PortChannel0001" @@ -396,9 +381,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1", "mtu": "9100", "name": "PortChannel0001" diff --git a/src/sonic-yang-models/tests/yang_model_tests/tests_config/vlan_sub_interface.json b/src/sonic-yang-models/tests/yang_model_tests/tests_config/vlan_sub_interface.json index b249510f4370..6f200ef4b7ea 100644 --- a/src/sonic-yang-models/tests/yang_model_tests/tests_config/vlan_sub_interface.json +++ b/src/sonic-yang-models/tests/yang_model_tests/tests_config/vlan_sub_interface.json @@ -196,9 +196,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1", "mtu": "9100", "tpid": "0x8100", @@ -246,9 +243,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1", "mtu": "9100", "tpid": "0x8100", @@ -296,9 +290,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1", "mtu": "9100", "tpid": "0x8100", @@ -346,9 +337,6 @@ "PORTCHANNEL_LIST": [ { "admin_status": "up", - "members": [ - "Ethernet0" - ], "min_links": "1", "mtu": "9100", "tpid": "0x8100", diff --git a/src/sonic-yang-models/yang-models/sonic-portchannel.yang b/src/sonic-yang-models/yang-models/sonic-portchannel.yang index 31235a0d2277..96872eeadb1d 100644 --- a/src/sonic-yang-models/yang-models/sonic-portchannel.yang +++ b/src/sonic-yang-models/yang-models/sonic-portchannel.yang @@ -58,23 +58,6 @@ module sonic-portchannel { } } - leaf-list members { - /* leaf-list members are unique by default */ - type union { - type leafref { - path /port:sonic-port/port:PORT/port:PORT_LIST/port:name; - } - type string { - pattern ""; - } - } - /* Today in SONiC, we do not delete the list once - * created, instead we set to empty list. Due to that - * below default values are needed. - */ - default ""; - } - leaf min_links { type uint16 { range 1..1024;