Skip to content

Commit

Permalink
[GCU] Supressing YANG errors from libyang while sorting (sonic-net#1991)
Browse files Browse the repository at this point in the history
#### What I did

Fixes sonic-net#2025

Stopping SonicYang from printing errors during config validation or moves generation.

If callers uses the `-v` option, they will see the SonicYang logs printed.

Also made sure that SonicYang returned error are properly propagated, so the user can see the exact errors.

#### How I did it
Used the the option `print_log_enabled` while creating `SonicYang`
```python
sy = sonic_yang.SonicYang(self.yang_dir, print_log_enabled=sonic_yang_print_log_enabled)
```
#### How to verify it
Manually tested the change to the output, and added unit-tests to the gu_common functions that were modified.

#### Previous command output (if the output of a command-line utility has changed)
Given an invalid patch will remove PORT table, while the PORT table is used.

```
admin@vlab-01:~$ sudo config apply-patch remove_port.json-patch -i /FEATURE
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "remove", "path": "/PORT"}]
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating target config does not have empty tables, since they do not show up in ConfigDb.
Patch Applier: Sorting patch updates.
sonic_yang(6):Note: Below table(s) have no YANG models: CONSOLE_SWITCH, DEVICE_NEIGHBOR_METADATA, DHCP_SERVER, KDUMP, RESTAPI, SNMP, SNMP_COMMUNITY, TELEMETRY
libyang[0]: Leafref "/sonic-port:sonic-port/sonic-port:PORT/sonic-port:PORT_LIST/sonic-port:name" of value "Ethernet112" points to a non-existing leaf. (path: /sonic-buffer-pg:sonic-buffer-pg/BUFFER_PG/BUFFER_PG_LIST[port='Ethernet112'][pg_num='0']/port)
libyang[0]: Leafref "/sonic-port:sonic-port/sonic-port:PORT/sonic-port:PORT_LIST/sonic-port:name" of value "Ethernet112" points to a non-existing leaf. (path: /sonic-buffer-pg:sonic-buffer-pg/BUFFER_PG/BUFFER_PG_LIST[port='Ethernet112'][pg_num='3-4']/port)
libyang[0]: Leafref "/sonic-port:sonic-port/sonic-port:PORT/sonic-port:PORT_LIST/sonic-port:name" of value "Ethernet116" points to a non-existing leaf. (path: /sonic-buffer-pg:sonic-buffer-pg/BUFFER_PG/BUFFER_PG_LIST[port='Ethernet116'][pg_num='0']/port)
>>>> 10s of log lines
libyang[0]: Leafref "/sonic-port:sonic-port/sonic-port:PORT/sonic-port:PORT_LIST/sonic-port:name" of value "Ethernet96" points to a non-existing leaf. (path: /sonic-port-qos-map:sonic-port-qos-map/PORT_QOS_MAP/PORT_QOS_MAP_LIST[ifname='Ethernet96']/ifname)
sonic_yang(3):Data Loading Failed:Leafref "/sonic-port:sonic-port/sonic-port:PORT/sonic-port:PORT_LIST/sonic-port:name" of value "Ethernet96" points to a non-existing leaf.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: Given patch is not valid because it will result in an invalid config
admin@vlab-01:~$ 
```
#### New command output (if the output of a command-line utility has changed)
```
admin@vlab-01:~$ sudo config apply-patch remove_port.json-patch -i /FEATURE
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "remove", "path": "/PORT"}]
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating target config does not have empty tables, since they do not show up in ConfigDb.
Patch Applier: Sorting patch updates.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: Given patch will produce invalid config. Error: Data Loading Failed
Leafref "/sonic-port:sonic-port/sonic-port:PORT/sonic-port:PORT_LIST/sonic-port:name" of value "Ethernet96" points to a non-existing leaf.
admin@vlab-01:~$ 
```
  • Loading branch information
ghooo authored Apr 29, 2022
1 parent fbfa8bc commit a54a091
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 19 deletions.
14 changes: 9 additions & 5 deletions generic_config_updater/gu_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ def validate_sonic_yang_config(self, sonic_yang_as_json):
sy.loadData(config_db_as_json)

sy.validate_data_tree()
return True
return True, None
except sonic_yang.SonicYangException as ex:
return False
return False, ex

def validate_config_db_config(self, config_db_as_json):
sy = self.create_sonic_yang_with_loaded_models()
Expand All @@ -118,9 +118,9 @@ def validate_config_db_config(self, config_db_as_json):
sy.loadData(tmp_config_db_as_json)

sy.validate_data_tree()
return True
return True, None
except sonic_yang.SonicYangException as ex:
return False
return False, ex

def crop_tables_without_yang(self, config_db_as_json):
sy = self.create_sonic_yang_with_loaded_models()
Expand Down Expand Up @@ -151,7 +151,8 @@ def remove_empty_tables(self, config):
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)
sonic_yang_print_log_enabled = genericUpdaterLogging.get_verbose()
loaded_models_sy = sonic_yang.SonicYang(self.yang_dir, print_log_enabled=sonic_yang_print_log_enabled)
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

Expand Down Expand Up @@ -829,6 +830,9 @@ def __init__(self):
def set_verbose(self, verbose):
self._verbose = verbose

def get_verbose(self):
return self._verbose

def get_logger(self, title, print_all_to_console=False):
return TitledLogger(SYSLOG_IDENTIFIER, title, self._verbose, print_all_to_console)

Expand Down
13 changes: 8 additions & 5 deletions generic_config_updater/patch_sorter.py
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,8 @@ def __init__(self, config_wrapper):

def validate(self, move, diff):
simulated_config = move.apply(diff.current_config)
return self.config_wrapper.validate_config_db_config(simulated_config)
is_valid, error = self.config_wrapper.validate_config_db_config(simulated_config)
return is_valid

# TODO: Add this validation to YANG models instead
class UniqueLanesMoveValidator:
Expand Down Expand Up @@ -1543,8 +1544,9 @@ def sort(self, patch, algorithm=Algorithm.DFS):

# Validate target config
self.logger.log_info("Validating target config according to YANG models.")
if not(self.config_wrapper.validate_config_db_config(target_config)):
raise ValueError(f"Given patch is not valid because it will result in an invalid config")
is_valid, error = self.config_wrapper.validate_config_db_config(target_config)
if not is_valid:
raise ValueError(f"Given patch will produce invalid config. Error: {error}")

# Generate list of changes to apply
self.logger.log_info("Sorting patch updates.")
Expand Down Expand Up @@ -1731,8 +1733,9 @@ def sort(self, patch, algorithm=Algorithm.DFS):

# Validate YANG covered target config
self.logger.log_info("Validating YANG covered target config according to YANG models.")
if not(self.config_wrapper.validate_config_db_config(target_config_yang)):
raise ValueError(f"Given patch is not valid because it will result in an invalid config")
is_valid, error = self.config_wrapper.validate_config_db_config(target_config_yang)
if not is_valid:
raise ValueError(f"Given patch will produce invalid config. Error: {error}")

# Generating changes associated with non-YANG covered configs
self.logger.log_info("Sorting non-YANG covered configs patch updates.")
Expand Down
12 changes: 8 additions & 4 deletions tests/generic_config_updater/gu_common_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,43 +144,47 @@ def test_validate_sonic_yang_config__valid_config__returns_true(self):
expected = True

# Act
actual = config_wrapper.validate_sonic_yang_config(Files.SONIC_YANG_AS_JSON)
actual, error = config_wrapper.validate_sonic_yang_config(Files.SONIC_YANG_AS_JSON)

# Assert
self.assertEqual(expected, actual)
self.assertIsNone(error)

def test_validate_sonic_yang_config__invvalid_config__returns_false(self):
# Arrange
config_wrapper = gu_common.ConfigWrapper()
expected = False

# Act
actual = config_wrapper.validate_sonic_yang_config(Files.SONIC_YANG_AS_JSON_INVALID)
actual, error = config_wrapper.validate_sonic_yang_config(Files.SONIC_YANG_AS_JSON_INVALID)

# Assert
self.assertEqual(expected, actual)
self.assertIsNotNone(error)

def test_validate_config_db_config__valid_config__returns_true(self):
# Arrange
config_wrapper = gu_common.ConfigWrapper()
expected = True

# Act
actual = config_wrapper.validate_config_db_config(Files.CONFIG_DB_AS_JSON)
actual, error = config_wrapper.validate_config_db_config(Files.CONFIG_DB_AS_JSON)

# Assert
self.assertEqual(expected, actual)
self.assertIsNone(error)

def test_validate_config_db_config__invalid_config__returns_false(self):
# Arrange
config_wrapper = gu_common.ConfigWrapper()
expected = False

# Act
actual = config_wrapper.validate_config_db_config(Files.CONFIG_DB_AS_JSON_INVALID)
actual, error = config_wrapper.validate_config_db_config(Files.CONFIG_DB_AS_JSON_INVALID)

# Assert
self.assertEqual(expected, actual)
self.assertIsNotNone(error)

def test_crop_tables_without_yang__returns_cropped_config_db_as_json(self):
# Arrange
Expand Down
11 changes: 6 additions & 5 deletions tests/generic_config_updater/patch_sorter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -925,7 +925,7 @@ def test_validate__invalid_config_db_after_applying_move__failure(self):
# Arrange
config_wrapper = Mock()
config_wrapper.validate_config_db_config.side_effect = \
create_side_effect_dict({(str(self.any_simulated_config),): False})
create_side_effect_dict({(str(self.any_simulated_config),): (False, None)})
validator = ps.FullConfigMoveValidator(config_wrapper)

# Act and assert
Expand All @@ -935,7 +935,7 @@ def test_validate__valid_config_db_after_applying_move__success(self):
# Arrange
config_wrapper = Mock()
config_wrapper.validate_config_db_config.side_effect = \
create_side_effect_dict({(str(self.any_simulated_config),): True})
create_side_effect_dict({(str(self.any_simulated_config),): (True, None)})
validator = ps.FullConfigMoveValidator(config_wrapper)

# Act and assert
Expand Down Expand Up @@ -3100,7 +3100,8 @@ def run_single_success_case(self, data, skip_exact_change_list_match):
simulated_config = current_config
for change in actual_changes:
simulated_config = change.apply(simulated_config)
self.assertTrue(self.config_wrapper.validate_config_db_config(simulated_config))
is_valid, error = self.config_wrapper.validate_config_db_config(simulated_config)
self.assertTrue(is_valid, f"Change will produce invalid config. Error: {error}")
self.assertEqual(target_config, simulated_config)

def test_patch_sorter_failure(self):
Expand Down Expand Up @@ -3424,7 +3425,7 @@ def __create_patch_sorter(self,
(str(any_target_config),): (any_target_config_yang, any_target_config_non_yang)})

config_wrapper.validate_config_db_config.side_effect = \
create_side_effect_dict({(str(any_target_config_yang),): valid_yang_covered_config})
create_side_effect_dict({(str(any_target_config_yang),): (valid_yang_covered_config, None)})

patch_wrapper.generate_patch.side_effect = \
create_side_effect_dict(
Expand Down Expand Up @@ -3517,7 +3518,7 @@ def __create_patch_sorter(self,

config_wrapper.validate_config_db_config.side_effect = \
create_side_effect_dict(
{(str(any_target_config),): valid_config_db})
{(str(any_target_config),): (valid_config_db, None)})


inner_patch_sorter.sort.side_effect = \
Expand Down

0 comments on commit a54a091

Please sign in to comment.