Skip to content

Commit

Permalink
[GCU] Using simulated config instead of target config when validating…
Browse files Browse the repository at this point in the history
… replace operation in NoDependencyMoveValidator (sonic-net#1987)

#### What I did
Using `SimulatedConfig` instead of `TargetConfig` for validating a move using `NoDependencyMoveValidator`

SimulatedConfig: config after applying the given move
TargetConfig: the final config state that's required by the patch

The problem is if the moves is to update a list item, the list item location in the `TargetConfig` might have different location in the `CurrentConfig`.

The reason for that is the `TargetConfig` has the final outcome after applying the `patch`, but the move might be just making a small change towards this goal.

Example:
Assume current_config
```
{
    "VLAN": {
        "Vlan100": {
            "vlanid": "100",
            "dhcp_servers": [
                "192.0.0.1",
                "192.0.0.2"
            ]
        }
    }
}
```
TargetConfig:
```
{
    "VLAN": {
        "Vlan100": {
            "vlanid": "100",
            "dhcp_servers": [
                "192.0.0.3"
            ]
        }
    }
}
```
Move:
```python
ps.JsonMove(diff, OperationType.REPLACE, ["VLAN", "Vlan100", "dhcp_servers", 1], ["VLAN", "Vlan100", "dhcp_servers", 0])
```


The move means:
```
Replace the value in CurrentConfig that has path `/VLAN/Vlan100/dhcp_servers/1` with
the value from the TargetConfig that has the path `/VLAN/Vlan100/dhcp_servers/0`
```
Notice how the array index in CurrentConfig does not exist in TargetConfig

Instead of using TargetConfig to validate, use SimulatedConfig which is the config after applying the move. In this case it would be:
```
{
    "VLAN": {
        "Vlan100": {
            "vlanid": "100",
            "dhcp_servers": [
                "192.0.0.1",
                "192.0.0.3"
            ]
        }
    }
}
```

#### How I did it
Replace `diff.target_config` with `simulated_config`

#### How to verify it
added a unit-test
  • Loading branch information
ghooo authored Dec 27, 2021
1 parent 35cb524 commit b3a5052
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 2 deletions.
6 changes: 4 additions & 2 deletions generic_config_updater/patch_sorter.py
Original file line number Diff line number Diff line change
Expand Up @@ -551,10 +551,12 @@ def _validate_replace(self, move, diff):
simulated_config = move.apply(diff.current_config)
deleted_paths, added_paths = self._get_paths(diff.current_config, simulated_config, [])

# For deleted paths, we check the current config has no dependencies between nodes under the removed path
if not self._validate_paths_config(deleted_paths, diff.current_config):
return False

if not self._validate_paths_config(added_paths, diff.target_config):
# For added paths, we check the simulated config has no dependencies between nodes under the added path
if not self._validate_paths_config(added_paths, simulated_config):
return False

return True
Expand Down Expand Up @@ -1020,7 +1022,7 @@ def __init__(self, move_wrapper):
self.move_wrapper = move_wrapper
self.mem = {}

def rec(self, diff):
def sort(self, diff):
if diff.has_no_diff():
return []

Expand Down
31 changes: 31 additions & 0 deletions tests/generic_config_updater/patch_sorter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1054,6 +1054,37 @@ def test_validate__replace_whole_config_item_same_ref_same__true(self):
# Act and assert
self.assertTrue(self.validator.validate(move, diff))

def test_validate__replace_list_item_different_location_than_target_and_no_deps__true(self):
# Arrange
current_config = {
"VLAN": {
"Vlan100": {
"vlanid": "100",
"dhcp_servers": [
"192.0.0.1",
"192.0.0.2"
]
}
}
}
target_config = {
"VLAN": {
"Vlan100": {
"vlanid": "100",
"dhcp_servers": [
"192.0.0.3"
]
}
}
}
diff = ps.Diff(current_config, target_config)
# the target tokens point to location 0 which exist in target_config
# but the replace operation is operating on location 1 in current_config
move = ps.JsonMove(diff, OperationType.REPLACE, ["VLAN", "Vlan100", "dhcp_servers", 1], ["VLAN", "Vlan100", "dhcp_servers", 0])

# Act and assert
self.assertTrue(self.validator.validate(move, diff))

def prepare_config(self, config, patch):
return patch.apply(config)

Expand Down

0 comments on commit b3a5052

Please sign in to comment.