From f5af780838f270c5df564735369d0438a9342a89 Mon Sep 17 00:00:00 2001 From: Mohamed Ghoneim Date: Wed, 25 May 2022 11:12:04 -0700 Subject: [PATCH] [GCU] Handling type1 lists (#2171) #### What I did Fixes #2034 Which lists where handled before in ConfigDb to YANG conversion? - Dictionary of key to Object e.g. ``` "TACPLUS_SERVER": { "1.1.1.1": { <= Key: Object (i.e. {...}) "priority": "1", "tcp_port": "49" }, "1.1.1.2": { "priority": "1", "tcp_port": "49" }, "1.1.1.3": { "priority": "1", "tcp_port": "49" } } ``` - List of values ``` "VLAN": { "Vlan1000": { "vlanid": "1000", "dhcp_servers": [ <= Values "192.0.0.1", "192.0.0.2", "192.0.0.3", "192.0.0.4" ] } } ``` - But there is no handling of Dictionary of Key to Value ``` "DOT1P_TO_TC_MAP": { "Dot1p_to_tc_map1": { <= Key: Value "1": "1", "2": "2", "3": "1", "4": "2" } }, ``` Refer to https://github.com/Azure/sonic-buildimage/pull/7375 for more info about how Type 1 list ConfigDb is getting converted to SonicYang. After checking how type1 is handled during ConfigDb to SonicYang conversion. Check the unit-tests here for converting ConfigDb Path to SonicYang xpath. Also added CABLE_LENGTH to ADD_RACK and REMOVE_RACK tests. #### How I did it When handling a list, check if it is of type1. If that's the case call type1 list handling. #### How to verify it unit-test #### Previous command output (if the output of a command-line utility has changed) #### New command output (if the output of a command-line utility has changed) --- generic_config_updater/gu_common.py | 91 ++++++++++++++++++- .../files/config_db_with_type1_tables.json | 22 +++++ .../files/patch_sorter_test_success.json | 49 +++++++++- .../generic_config_updater/gu_common_test.py | 27 ++++++ .../patch_sorter_test.py | 1 - 5 files changed, 181 insertions(+), 9 deletions(-) create mode 100644 tests/generic_config_updater/files/config_db_with_type1_tables.json diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index c0206cf198..fb334c17db 100644 --- a/generic_config_updater/gu_common.py +++ b/generic_config_updater/gu_common.py @@ -2,6 +2,7 @@ import jsonpatch from jsonpointer import JsonPointer import sonic_yang +import sonic_yang_ext import subprocess import yang as ly import copy @@ -155,14 +156,14 @@ def crop_tables_without_yang(self, config_db_as_json): sy._cropConfigDB() return sy.jIn - + def get_empty_tables(self, config): empty_tables = [] for key in config.keys(): if not(config[key]): empty_tables.append(key) return empty_tables - + def remove_empty_tables(self, config): config_with_non_empty_tables = {} for table in config: @@ -398,7 +399,7 @@ def find_ref_paths(self, path, config): Finds the paths referencing any line under the given 'path' within the given 'config'. Example: path: /PORT - config: + config: { "VLAN_MEMBER": { "Vlan1000|Ethernet0": {}, @@ -543,10 +544,25 @@ def _get_xpath_tokens_from_list(self, model, token_index, path_tokens, config): if len(path_tokens)-1 == token_index: return xpath_tokens + type_1_list_model = self._get_type_1_list_model(model) + if type_1_list_model: + new_xpath_tokens = self._get_xpath_tokens_from_type_1_list(type_1_list_model, token_index+1, path_tokens, config[path_tokens[token_index]]) + xpath_tokens.extend(new_xpath_tokens) + return xpath_tokens + new_xpath_tokens = self._get_xpath_tokens_from_leaf(model, token_index+1, path_tokens,config[path_tokens[token_index]]) xpath_tokens.extend(new_xpath_tokens) return xpath_tokens + def _get_xpath_tokens_from_type_1_list(self, model, token_index, path_tokens, config): + type_1_list_name = model['@name'] + keyName = model['key']['@value'] + value = path_tokens[token_index] + keyToken = f"[{keyName}='{value}']" + itemToken = f"{type_1_list_name}{keyToken}" + + return [itemToken] + def _get_xpath_tokens_from_leaf(self, model, token_index, path_tokens, config): token = path_tokens[token_index] @@ -580,7 +596,7 @@ def _get_xpath_tokens_from_leaf(self, model, token_index, path_tokens, config): # /module-name:container/leaf-list[.='val'] # Source: Check examples in https://netopeer.liberouter.org/doc/libyang/master/html/howto_x_path.html return [f"{token}[.='{value}']"] - + # checking 'uses' statement if not isinstance(config[token], list): # leaf-list under uses is not supported yet in sonic_yang table = path_tokens[0] @@ -608,7 +624,7 @@ def _extractKey(self, tableKey, keys): def _get_list_model(self, model, token_index, path_tokens): parent_container_name = path_tokens[token_index] clist = model.get('list') - # Container contains a single list, just return it + # Container contains a single list, just return it # TODO: check if matching also by name is necessary if isinstance(clist, dict): return clist @@ -630,6 +646,15 @@ def _get_list_model(self, model, token_index, path_tokens): return None + def _get_type_1_list_model(self, model): + list_name = model['@name'] + if list_name not in sonic_yang_ext.Type_1_list_maps_model: + return None + + # Type 1 list is expected to have a single inner list model. + # No need to check if it is a dictionary of list models. + return model.get('list') + def convert_xpath_to_path(self, xpath, config, sy): """ Converts the given XPATH to JsonPatch path (i.e. JsonPointer). @@ -711,10 +736,66 @@ def _get_path_tokens_from_list(self, model, token_index, xpath_tokens, config): if next_token in key_dict: return path_tokens + type_1_list_model = self._get_type_1_list_model(model) + if type_1_list_model: + new_path_tokens = self._get_path_tokens_from_type_1_list(type_1_list_model, token_index+1, xpath_tokens, config[path_token]) + path_tokens.extend(new_path_tokens) + return path_tokens + new_path_tokens = self._get_path_tokens_from_leaf(model, token_index+1, xpath_tokens, config[path_token]) path_tokens.extend(new_path_tokens) return path_tokens + def _get_path_tokens_from_type_1_list(self, model, token_index, xpath_tokens, config): + type_1_inner_list_name = model['@name'] + + token = xpath_tokens[token_index] + list_tokens = token.split("[", 1) # split once on the first '[', first element will be the inner list name + inner_list_name = list_tokens[0] + + if type_1_inner_list_name != inner_list_name: + raise GenericConfigUpdaterError(f"Type 1 inner list name '{type_1_inner_list_name}' does match xpath inner list name '{inner_list_name}'.") + + key_dict = self._extract_key_dict(token) + + # If no keys specified return empty tokens, as we are already inside the correct table. + # Also note that the type 1 inner list name in SonicYang has no correspondence in ConfigDb and is ignored. + # Example where VLAN_MEMBER_LIST has no specific key/value: + # xpath: /sonic-dot1p-tc-map:sonic-dot1p-tc-map/DOT1P_TO_TC_MAP/DOT1P_TO_TC_MAP_LIST[name='Dot1p_to_tc_map1']/DOT1P_TO_TC_MAP + # path: /DOT1P_TO_TC_MAP/Dot1p_to_tc_map1 + if not(key_dict): + return [] + + if len(key_dict) > 1: + raise GenericConfigUpdaterError(f"Type 1 inner list should have only 1 key in xpath, {len(key_dict)} specified. Key dictionary: {key_dict}") + + keyName = next(iter(key_dict.keys())) + value = key_dict[keyName] + + path_tokens = [value] + + # If this is the last xpath token, return the path tokens we have built so far, no need for futher checks + # Example: + # xpath: /sonic-dot1p-tc-map:sonic-dot1p-tc-map/DOT1P_TO_TC_MAP/DOT1P_TO_TC_MAP_LIST[name='Dot1p_to_tc_map1']/DOT1P_TO_TC_MAP[dot1p='2'] + # path: /DOT1P_TO_TC_MAP/Dot1p_to_tc_map1/2 + if token_index+1 >= len(xpath_tokens): + return path_tokens + + # Checking if the next_token is actually a child leaf of the inner type 1 list, for which case + # just ignore the token, and return the already created ConfigDb path pointing to the whole object + # Example where the leaf specified is the key: + # xpath: /sonic-dot1p-tc-map:sonic-dot1p-tc-map/DOT1P_TO_TC_MAP/DOT1P_TO_TC_MAP_LIST[name='Dot1p_to_tc_map1']/DOT1P_TO_TC_MAP[dot1p='2']/dot1p + # path: /DOT1P_TO_TC_MAP/Dot1p_to_tc_map1/2 + # Example where the leaf specified is not the key: + # xpath: /sonic-dot1p-tc-map:sonic-dot1p-tc-map/DOT1P_TO_TC_MAP/DOT1P_TO_TC_MAP_LIST[name='Dot1p_to_tc_map1']/DOT1P_TO_TC_MAP[dot1p='2']/tc + # path: /DOT1P_TO_TC_MAP/Dot1p_to_tc_map1/2 + next_token = xpath_tokens[token_index+1] + leaf_model = self._get_model(model.get('leaf'), next_token) + if leaf_model: + return path_tokens + + raise GenericConfigUpdaterError(f"Type 1 inner list '{type_1_inner_list_name}' does not have a child leaf named '{next_token}'") + def _get_path_tokens_from_leaf(self, model, token_index, xpath_tokens, config): token = xpath_tokens[token_index] diff --git a/tests/generic_config_updater/files/config_db_with_type1_tables.json b/tests/generic_config_updater/files/config_db_with_type1_tables.json new file mode 100644 index 0000000000..c1f2d3e389 --- /dev/null +++ b/tests/generic_config_updater/files/config_db_with_type1_tables.json @@ -0,0 +1,22 @@ +{ + "DOT1P_TO_TC_MAP": { + "Dot1p_to_tc_map1": { + "1": "1", + "2": "2" + }, + "Dot1p_to_tc_map2": { + "3": "3", + "4": "4" + } + }, + "EXP_TO_FC_MAP": { + "Exp_to_fc_map1": { + "1": "1", + "2": "2" + }, + "Exp_to_fc_map2": { + "3": "3", + "4": "4" + } + } +} \ No newline at end of file diff --git a/tests/generic_config_updater/files/patch_sorter_test_success.json b/tests/generic_config_updater/files/patch_sorter_test_success.json index 74cb4ed728..b4f1f141c3 100644 --- a/tests/generic_config_updater/files/patch_sorter_test_success.json +++ b/tests/generic_config_updater/files/patch_sorter_test_success.json @@ -2673,8 +2673,10 @@ ] }, "ADDING_LOOPBACK0_VRF_NAME__DELETES_LOOPBACK0_AND_IPS_DOES_NOT_AFFECT_OTHER_TABLES": { - "desc": ["Adding loopback vrf name, deletes loopback0 and the associated ips. ", - "It does not affect other tables."], + "desc": [ + "Adding loopback vrf name, deletes loopback0 and the associated ips. ", + "It does not affect other tables." + ], "current_config": { "CABLE_LENGTH": { "AZURE": { @@ -2733,7 +2735,7 @@ "op": "add", "path": "/LOOPBACK_INTERFACE", "value": { - "Loopback0":{ + "Loopback0": { "vrf_name": "Vrf_01" } } @@ -3538,6 +3540,15 @@ "profile": "egress_lossy_profile" } }, + "CABLE_LENGTH": { + "AZURE": { + "Ethernet52": "40m", + "Ethernet56": "40m", + "Ethernet60": "40m", + "Ethernet68": "40m", + "Ethernet72": "40m" + } + }, "DEVICE_NEIGHBOR": { "Ethernet52": { "name": "ARISTA13T2", @@ -3819,6 +3830,11 @@ "path": "/ACL_TABLE/EVERFLOW/ports/0", "value": "Ethernet64" }, + { + "op": "add", + "path": "/CABLE_LENGTH/AZURE/Ethernet64", + "value": "40m" + }, { "op": "add", "path": "/INTERFACE/Ethernet64", @@ -4584,6 +4600,13 @@ "value": "up" } ], + [ + { + "op": "add", + "path": "/CABLE_LENGTH/AZURE/Ethernet64", + "value": "40m" + } + ], [ { "op": "replace", @@ -5233,6 +5256,16 @@ "profile": "egress_lossy_profile" } }, + "CABLE_LENGTH": { + "AZURE": { + "Ethernet52": "40m", + "Ethernet56": "40m", + "Ethernet60": "40m", + "Ethernet64": "40m", + "Ethernet68": "40m", + "Ethernet72": "40m" + } + }, "DEVICE_NEIGHBOR": { "Ethernet52": { "name": "ARISTA13T2", @@ -5680,6 +5713,10 @@ "op": "remove", "path": "/BGP_NEIGHBOR/fc00::7e/admin_status" }, + { + "op": "remove", + "path": "/CABLE_LENGTH/AZURE/Ethernet64" + }, { "op": "remove", "path": "/INTERFACE/Ethernet64" @@ -6063,6 +6100,12 @@ "path": "/BGP_NEIGHBOR/fc00::a/admin_status" } ], + [ + { + "op": "remove", + "path": "/CABLE_LENGTH/AZURE/Ethernet64" + } + ], [ { "op": "replace", diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py index 2beb2a2987..8902df649a 100644 --- a/tests/generic_config_updater/gu_common_test.py +++ b/tests/generic_config_updater/gu_common_test.py @@ -855,6 +855,15 @@ def check(path, xpath, config=None): check(path="/BUFFER_PORT_EGRESS_PROFILE_LIST/Ethernet9/profile_list", xpath="/sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list/BUFFER_PORT_EGRESS_PROFILE_LIST/BUFFER_PORT_EGRESS_PROFILE_LIST_LIST[port='Ethernet9']/profile_list", config=Files.CONFIG_DB_WITH_PROFILE_LIST) + check(path="/DOT1P_TO_TC_MAP/Dot1p_to_tc_map1", + xpath="/sonic-dot1p-tc-map:sonic-dot1p-tc-map/DOT1P_TO_TC_MAP/DOT1P_TO_TC_MAP_LIST[name='Dot1p_to_tc_map1']", + config=Files.CONFIG_DB_WITH_TYPE1_TABLES) + check(path="/DOT1P_TO_TC_MAP/Dot1p_to_tc_map1/2", + xpath="/sonic-dot1p-tc-map:sonic-dot1p-tc-map/DOT1P_TO_TC_MAP/DOT1P_TO_TC_MAP_LIST[name='Dot1p_to_tc_map1']/DOT1P_TO_TC_MAP[dot1p='2']", + config=Files.CONFIG_DB_WITH_TYPE1_TABLES) + check(path="/EXP_TO_FC_MAP/Exp_to_fc_map2/4", + xpath="/sonic-exp-fc-map:sonic-exp-fc-map/EXP_TO_FC_MAP/EXP_TO_FC_MAP_LIST[name='Exp_to_fc_map2']/EXP_TO_FC_MAP[exp='4']", + config=Files.CONFIG_DB_WITH_TYPE1_TABLES) def test_convert_xpath_to_path(self): def check(xpath, path, config=None): @@ -936,6 +945,24 @@ def check(xpath, path, config=None): check(xpath="/sonic-buffer-port-egress-profile-list:sonic-buffer-port-egress-profile-list/BUFFER_PORT_EGRESS_PROFILE_LIST/BUFFER_PORT_EGRESS_PROFILE_LIST_LIST[port='Ethernet9']/profile_list[.='egress_lossy_profile']", path="/BUFFER_PORT_EGRESS_PROFILE_LIST/Ethernet9/profile_list", config=Files.CONFIG_DB_WITH_PROFILE_LIST) + check(xpath="/sonic-dot1p-tc-map:sonic-dot1p-tc-map/DOT1P_TO_TC_MAP/DOT1P_TO_TC_MAP_LIST[name='Dot1p_to_tc_map1']", + path="/DOT1P_TO_TC_MAP/Dot1p_to_tc_map1", + config=Files.CONFIG_DB_WITH_TYPE1_TABLES) + check(xpath="/sonic-dot1p-tc-map:sonic-dot1p-tc-map/DOT1P_TO_TC_MAP/DOT1P_TO_TC_MAP_LIST[name='Dot1p_to_tc_map1']/DOT1P_TO_TC_MAP", + path="/DOT1P_TO_TC_MAP/Dot1p_to_tc_map1", + config=Files.CONFIG_DB_WITH_TYPE1_TABLES) + check(xpath="/sonic-dot1p-tc-map:sonic-dot1p-tc-map/DOT1P_TO_TC_MAP/DOT1P_TO_TC_MAP_LIST[name='Dot1p_to_tc_map1']/DOT1P_TO_TC_MAP[dot1p='2']", + path="/DOT1P_TO_TC_MAP/Dot1p_to_tc_map1/2", + config=Files.CONFIG_DB_WITH_TYPE1_TABLES) + check(xpath="/sonic-dot1p-tc-map:sonic-dot1p-tc-map/DOT1P_TO_TC_MAP/DOT1P_TO_TC_MAP_LIST[name='Dot1p_to_tc_map1']/DOT1P_TO_TC_MAP[dot1p='2']/dot1p", + path="/DOT1P_TO_TC_MAP/Dot1p_to_tc_map1/2", + config=Files.CONFIG_DB_WITH_TYPE1_TABLES) + check(xpath="/sonic-dot1p-tc-map:sonic-dot1p-tc-map/DOT1P_TO_TC_MAP/DOT1P_TO_TC_MAP_LIST[name='Dot1p_to_tc_map1']/DOT1P_TO_TC_MAP[dot1p='2']/tc", + path="/DOT1P_TO_TC_MAP/Dot1p_to_tc_map1/2", + config=Files.CONFIG_DB_WITH_TYPE1_TABLES) + check(xpath="/sonic-exp-fc-map:sonic-exp-fc-map/EXP_TO_FC_MAP/EXP_TO_FC_MAP_LIST[name='Exp_to_fc_map2']/EXP_TO_FC_MAP[exp='4']", + path="/EXP_TO_FC_MAP/Exp_to_fc_map2/4", + config=Files.CONFIG_DB_WITH_TYPE1_TABLES) def test_has_path(self): def check(config, path, expected): diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py index 4cb8fa7019..ce4e1a3a13 100644 --- a/tests/generic_config_updater/patch_sorter_test.py +++ b/tests/generic_config_updater/patch_sorter_test.py @@ -3078,7 +3078,6 @@ def test_patch_sorter_success(self): data = Files.PATCH_SORTER_TEST_SUCCESS skip_exact_change_list_match = False for test_case_name in data: - # TODO: Add CABLE_LENGTH to ADD_RACK and REMOVE_RACK tests https://github.com/Azure/sonic-utilities/issues/2034 with self.subTest(name=test_case_name): self.run_single_success_case(data[test_case_name], skip_exact_change_list_match)