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.

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] Loading yang-models only once #1981

Merged
merged 3 commits into from
Dec 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
35 changes: 22 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,16 @@ def remove_empty_tables(self, config):
config_with_non_empty_tables[table] = copy.deepcopy(config[table])
return config_with_non_empty_tables

# TODO: move creating copies of sonic_yang with loaded models to sonic-yang-mgmt directly
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() # This call takes a long time (100s of ms) because it reads files from disk
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


return copy.copy(self.sonic_yang_with_loaded_models)

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 +182,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 +262,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 +401,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
45 changes: 44 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,49 @@ 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 or None, non-yang model fields are different
def check(sy1, sy2):
# instances are different
self.assertNotEqual(sy1, sy2)

# yang models fields are same or None
self.assertTrue(sy1.confDbYangMap is sy2.confDbYangMap)
self.assertTrue(sy1.ctx is sy2.ctx)
self.assertTrue(sy1.DEBUG is sy2.DEBUG)
self.assertTrue(sy1.preProcessedYang is sy2.preProcessedYang)
self.assertTrue(sy1.SYSLOG_IDENTIFIER is sy2.SYSLOG_IDENTIFIER)
self.assertTrue(sy1.yang_dir is sy2.yang_dir)
self.assertTrue(sy1.yangFiles is sy2.yangFiles)
self.assertTrue(sy1.yJson is sy2.yJson)
self.assertTrue(not(hasattr(sy1, 'module')) or sy1.module is None) # module is unused, might get deleted
self.assertTrue(not(hasattr(sy2, 'module')) or sy2.module is None)

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

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()

# Simulating loading non-yang model fields
sy1.loadData(Files.ANY_CONFIG_DB)
sy1.getData()

# Simulating loading non-yang model fields
sy2.loadData(Files.ANY_CONFIG_DB)
sy2.getData()

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 +486,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