Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GCU] Handling type1 lists #2171

Merged
merged 5 commits into from
May 25, 2022
Merged

[GCU] Handling type1 lists #2171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6d77480
[GCU] Handling type1 lists
ghooo May 18, 2022
d185221
updating ADD_RACK and REMOVE_RACK test cases
ghooo May 18, 2022
0ba1a8e
addressing comments
ghooo May 18, 2022
5f48c97
removing unused variable
ghooo May 18, 2022
663ae2b
minor fix
ghooo May 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 86 additions & 5 deletions generic_config_updater/gu_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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": {},
Expand Down Expand Up @@ -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]

Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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
Expand All @@ -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).
Expand Down Expand Up @@ -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]
Copy link
Contributor

@qiluo-msft qiluo-msft May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

token_index

Protect for index out of range. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is protected in line 777

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me add a comment to line 777 for clarification, and to indicate that it is the end of the xpath_tokens

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about token_index+1 > len(xpath_tokens)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

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]

Expand Down
22 changes: 22 additions & 0 deletions tests/generic_config_updater/files/config_db_with_type1_tables.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
}
}
}
49 changes: 46 additions & 3 deletions tests/generic_config_updater/files/patch_sorter_test_success.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down Expand Up @@ -2733,7 +2735,7 @@
"op": "add",
"path": "/LOOPBACK_INTERFACE",
"value": {
"Loopback0":{
"Loopback0": {
"vrf_name": "Vrf_01"
}
}
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -4584,6 +4600,13 @@
"value": "up"
}
],
[
{
"op": "add",
"path": "/CABLE_LENGTH/AZURE/Ethernet64",
"value": "40m"
}
],
[
{
"op": "replace",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -6063,6 +6100,12 @@
"path": "/BGP_NEIGHBOR/fc00::a/admin_status"
}
],
[
{
"op": "remove",
"path": "/CABLE_LENGTH/AZURE/Ethernet64"
}
],
[
{
"op": "replace",
Expand Down
27 changes: 27 additions & 0 deletions tests/generic_config_updater/gu_common_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down
1 change: 0 additions & 1 deletion tests/generic_config_updater/patch_sorter_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down