Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
43 changes: 30 additions & 13 deletions generic_config_updater/gu_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ def __eq__(self, other):
class ConfigWrapper:
def __init__(self, yang_dir = YANG_DIR):
self.yang_dir = YANG_DIR
self.sonic_yang_with_loaded_models = None
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 21, 2021

Choose a reason for hiding this comment

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

sonic_yang_with_loaded_models

Is sonic_yang_model good enough? #Closed

Copy link
Contributor Author

@ghooo ghooo Dec 21, 2021

Choose a reason for hiding this comment

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

I don't think so. sonic_yang_model refers to the model but here we are referring to SonicYang object check code

This variable will hold an instance of SonicYang but with the models loaded hence the name


def get_config_db_as_json(self):
text = self._get_config_db_as_text()
Expand All @@ -63,8 +64,7 @@ def get_sonic_yang_as_json(self):
return self.convert_config_db_to_sonic_yang(config_db_json)

def convert_config_db_to_sonic_yang(self, config_db_as_json):
sy = sonic_yang.SonicYang(self.yang_dir)
sy.loadYangModel()
sy = self.create_sonic_yang_with_loaded_models()

# Crop config_db tables that do not have sonic yang models
cropped_config_db_as_json = self.crop_tables_without_yang(config_db_as_json)
Expand All @@ -76,8 +76,7 @@ def convert_config_db_to_sonic_yang(self, config_db_as_json):
return sonic_yang_as_json

def convert_sonic_yang_to_config_db(self, sonic_yang_as_json):
sy = sonic_yang.SonicYang(self.yang_dir)
sy.loadYangModel()
sy = self.create_sonic_yang_with_loaded_models()

# replace container of the format 'module:table' with just 'table'
new_sonic_yang_json = {}
Expand All @@ -100,8 +99,7 @@ def convert_sonic_yang_to_config_db(self, sonic_yang_as_json):
def validate_sonic_yang_config(self, sonic_yang_as_json):
config_db_as_json = self.convert_sonic_yang_to_config_db(sonic_yang_as_json)

sy = sonic_yang.SonicYang(self.yang_dir)
sy.loadYangModel()
sy = self.create_sonic_yang_with_loaded_models()

try:
sy.loadData(config_db_as_json)
Expand All @@ -112,8 +110,7 @@ def validate_sonic_yang_config(self, sonic_yang_as_json):
return False

def validate_config_db_config(self, config_db_as_json):
sy = sonic_yang.SonicYang(self.yang_dir)
sy.loadYangModel()
sy = self.create_sonic_yang_with_loaded_models()

try:
tmp_config_db_as_json = copy.deepcopy(config_db_as_json)
Expand All @@ -126,8 +123,7 @@ def validate_config_db_config(self, config_db_as_json):
return False

def crop_tables_without_yang(self, config_db_as_json):
sy = sonic_yang.SonicYang(self.yang_dir)
sy.loadYangModel()
sy = self.create_sonic_yang_with_loaded_models()

sy.jIn = copy.deepcopy(config_db_as_json)

Expand All @@ -151,6 +147,24 @@ def remove_empty_tables(self, config):
config_with_non_empty_tables[table] = copy.deepcopy(config[table])
return config_with_non_empty_tables

def create_sonic_yang_with_loaded_models(self):
# sonic_yang_with_loaded_models will only be initialized once the first time this method is called
if self.sonic_yang_with_loaded_models is None:
loaded_models_sy = sonic_yang.SonicYang(self.yang_dir)
loaded_models_sy.loadYangModel()
self.sonic_yang_with_loaded_models = loaded_models_sy
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 21, 2021

Choose a reason for hiding this comment

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

Could you add comment which line spends most of the time? #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.

ok

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


# create a copy of sonic_yang_with_loaded_models, only copy the loaded yang models related properties
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 21, 2021

Choose a reason for hiding this comment

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

create

This is similar to shallow copy. Did you consider https://docs.python.org/3/library/copy.html#copy.copy? #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.

Good idea

# TODO: move to sonic-yang-mgmt directly
sy = sonic_yang.SonicYang(self.yang_dir)
sy.confDbYangMap = self.sonic_yang_with_loaded_models.confDbYangMap
sy.ctx = self.sonic_yang_with_loaded_models.ctx
sy.preProcessedYang = self.sonic_yang_with_loaded_models.preProcessedYang
sy.yangFiles = self.sonic_yang_with_loaded_models.yangFiles
sy.yJson = self.sonic_yang_with_loaded_models.yJson

return sy

class DryRunConfigWrapper(ConfigWrapper):
# This class will simulate all read/write operations to ConfigDB on a virtual storage unit.
def __init__(self, initial_imitated_config_db = None):
Expand All @@ -176,7 +190,7 @@ def _init_imitated_config_db_if_none(self):
class PatchWrapper:
def __init__(self, config_wrapper=None):
self.config_wrapper = config_wrapper if config_wrapper is not None else ConfigWrapper()
self.path_addressing = PathAddressing()
self.path_addressing = PathAddressing(self.config_wrapper)

def validate_config_db_patch_has_yang_models(self, patch):
config_db = {}
Expand Down Expand Up @@ -256,6 +270,10 @@ class PathAddressing:
"""
PATH_SEPARATOR = "/"
XPATH_SEPARATOR = "/"

def __init__(self, config_wrapper=None):
self.config_wrapper = config_wrapper

def get_path_tokens(self, path):
return JsonPointer(path).parts

Expand Down Expand Up @@ -391,8 +409,7 @@ def find_ref_paths(self, path, config):
return self._find_leafref_paths(path, config)

def _find_leafref_paths(self, path, config):
sy = sonic_yang.SonicYang(YANG_DIR)
sy.loadYangModel()
sy = self.config_wrapper.create_sonic_yang_with_loaded_models()

sy.loadData(config)

Expand Down
8 changes: 4 additions & 4 deletions generic_config_updater/patch_sorter.py
Original file line number Diff line number Diff line change
Expand Up @@ -1119,21 +1119,21 @@ class IgnorePathsFromYangConfigSplitter:
def __init__(self, ignore_paths_from_yang_list, config_wrapper):
self.ignore_paths_from_yang_list = ignore_paths_from_yang_list
self.config_wrapper = config_wrapper
self.path_addressing = PathAddressing(config_wrapper)

def split_yang_non_yang_distinct_field_path(self, config):
config_with_yang = copy.deepcopy(config)
config_without_yang = {}

path_addressing = PathAddressing()
# ignore more config from config_with_yang
for path in self.ignore_paths_from_yang_list:
if not path_addressing.has_path(config_with_yang, path):
if not self.path_addressing.has_path(config_with_yang, path):
continue
if path == '': # whole config to be ignored
return {}, copy.deepcopy(config)

# Add to config_without_yang from config_with_yang
tokens = path_addressing.get_path_tokens(path)
tokens = self.path_addressing.get_path_tokens(path)
add_move = JsonMove(Diff(config_without_yang, config_with_yang), OperationType.ADD, tokens, tokens)
config_without_yang = add_move.apply(config_without_yang)

Expand Down Expand Up @@ -1325,7 +1325,7 @@ def __init__(self, config_wrapper, patch_wrapper, sort_algorithm_factory=None):
self.config_wrapper = config_wrapper
self.patch_wrapper = patch_wrapper
self.operation_wrapper = OperationWrapper()
self.path_addressing = PathAddressing()
self.path_addressing = PathAddressing(self.config_wrapper)
self.sort_algorithm_factory = sort_algorithm_factory if sort_algorithm_factory else \
SortAlgorithmFactory(self.operation_wrapper, config_wrapper, self.path_addressing)

Expand Down
31 changes: 30 additions & 1 deletion tests/generic_config_updater/gu_common_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,35 @@ def test_remove_empty_tables__multiple_empty_tables__returns_config_without_empt
# Assert
self.assertDictEqual({"any_table": {"key": "value"}}, actual)

def test_create_sonic_yang_with_loaded_models__creates_new_sonic_yang_every_call(self):
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 21, 2021

Choose a reason for hiding this comment

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

test_create_sonic_yang_with_loaded_models__creates_new_sonic_yang_every_call

You mentioned

TestPatchSorter took ... Before: 800s

The performance comparison is interesting. Could you explain what happens in 800s? How can we repro your result by unit test? #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.

updated pr description with profiling data for a specific case instead of all cases in TestPatchSorter

# check yang models fields are the same, non-yang model fields are different
def check(sy1, sy2):
# instances are different
self.assertNotEqual(sy1, sy2)

# yang models fields are same
self.assertTrue(sy1.confDbYangMap is sy2.confDbYangMap)
self.assertTrue(sy1.ctx is sy2.ctx)
self.assertTrue(sy1.preProcessedYang is sy2.preProcessedYang)
self.assertTrue(sy1.yangFiles is sy2.yangFiles)
self.assertTrue(sy1.yJson is sy2.yJson)

# non yang models fields are different
self.assertFalse(sy1.jIn is sy2.jIn)
self.assertFalse(sy1.xlateJson is sy2.xlateJson)
self.assertFalse(sy1.revXlateJson is sy2.revXlateJson)
self.assertFalse(sy1.tablesWithOutYang is sy2.tablesWithOutYang)

config_wrapper = gu_common.ConfigWrapper()
self.assertTrue(config_wrapper.sonic_yang_with_loaded_models is None)

sy1 = config_wrapper.create_sonic_yang_with_loaded_models()
sy2 = config_wrapper.create_sonic_yang_with_loaded_models()

check(sy1, sy2)
check(sy1, config_wrapper.sonic_yang_with_loaded_models)
check(sy2, config_wrapper.sonic_yang_with_loaded_models)

class TestPatchWrapper(unittest.TestCase):
def setUp(self):
self.config_wrapper_mock = gu_common.ConfigWrapper()
Expand Down Expand Up @@ -443,7 +472,7 @@ def __assert_same_patch(self, config_db_patch, sonic_yang_patch, config_wrapper,

class TestPathAddressing(unittest.TestCase):
def setUp(self):
self.path_addressing = gu_common.PathAddressing()
self.path_addressing = gu_common.PathAddressing(gu_common.ConfigWrapper())
self.sy_only_models = sonic_yang.SonicYang(gu_common.YANG_DIR)
self.sy_only_models.loadYangModel()

Expand Down
9 changes: 5 additions & 4 deletions tests/generic_config_updater/patch_sorter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -921,8 +921,8 @@ def verify_diff(self, current_config, target_config, current_config_tokens=None,

class TestNoDependencyMoveValidator(unittest.TestCase):
def setUp(self):
path_addressing = ps.PathAddressing()
config_wrapper = ConfigWrapper()
path_addressing = ps.PathAddressing(config_wrapper)
self.validator = ps.NoDependencyMoveValidator(path_addressing, config_wrapper)

def test_validate__add_full_config_has_dependencies__failure(self):
Expand Down Expand Up @@ -1649,7 +1649,7 @@ def verify_moves(self, ex_ops, moves):

class DeleteRefsMoveExtender(unittest.TestCase):
def setUp(self):
self.extender = ps.DeleteRefsMoveExtender(PathAddressing())
self.extender = ps.DeleteRefsMoveExtender(PathAddressing(ConfigWrapper()))

def test_extend__non_delete_ops__no_extended_moves(self):
self.verify(OperationType.ADD,
Expand Down Expand Up @@ -1723,7 +1723,8 @@ def test_memoization_sorter(self):

def verify(self, algo, algo_class):
# Arrange
factory = ps.SortAlgorithmFactory(OperationWrapper(), ConfigWrapper(), PathAddressing())
config_wrapper = ConfigWrapper()
factory = ps.SortAlgorithmFactory(OperationWrapper(), config_wrapper, PathAddressing(config_wrapper))
expected_generators = [ps.LowLevelMoveGenerator]
expected_extenders = [ps.UpperLevelMoveExtender, ps.DeleteInsteadOfReplaceMoveExtender, ps.DeleteRefsMoveExtender]
expected_validator = [ps.DeleteWholeConfigMoveValidator,
Expand Down Expand Up @@ -1753,7 +1754,7 @@ def create_patch_sorter(self, config=None):
config_wrapper.get_config_db_as_json = MagicMock(return_value=config)
patch_wrapper = PatchWrapper(config_wrapper)
operation_wrapper = OperationWrapper()
path_addressing= ps.PathAddressing()
path_addressing= ps.PathAddressing(config_wrapper)
sort_algorithm_factory = ps.SortAlgorithmFactory(operation_wrapper, config_wrapper, path_addressing)

return ps.PatchSorter(config_wrapper, patch_wrapper, sort_algorithm_factory)
Expand Down