[config][generic-update] Implement ChangeApplier for ConfigDb format #1763
Conversation
| @@ -18,9 +21,94 @@ def release_lock(self): | |||
| pass | |||
|
|
|||
| class ChangeApplier: | |||
There was a problem hiding this comment.
maybe move it to a new file? #Closed
There was a problem hiding this comment.
will move in later iteration.
| rc = self.exec_command(restart_comand) | ||
| if rc != 0: | ||
| raise GenericConfigUpdaterError(f"Restart command failed: {restart_comand}, rc={rc}") | ||
| rc = self.exec_command(validate_command) |
There was a problem hiding this comment.
Validate commands do not need to be associated with restart commands, i.e. we should be able to verify the services that absorb the changes automatically. #Resolved
| self.config_db.set_entry(parts[0], parts[1], entry) | ||
|
|
||
| def apply(self, change: JsonChange): | ||
| patch = change.patch |
There was a problem hiding this comment.
Using the patch within the change is ok, but it is limiting and is a little different than the design document. Using patch will forces specific order of execution on the ChangeApplier while in fact there is no ordering. This causes problems like multiple restarts which is solved by the lazy restart.
JsonChange on the other hand can be used to just getting the target config, and then the diff. The diff is per table, so we can handle table by table alphabetically.
JsonChange only provides one function which is apply (notice the second step in the diagram) GenerateTargetConfig. I think patch access should be reduced i.e. to be preceded by _
| rc = self.exec_command(restart_comand) | ||
| if rc != 0: | ||
| raise GenericConfigUpdaterError(f"Restart command failed: {restart_comand}, rc={rc}") | ||
| rc = self.exec_command(validate_command) |
There was a problem hiding this comment.
Validate commands expect ConfigDb before and after the update, from design doc:
validate-commands - Multiple CLI commands used to validate the service has absorbed the change successfully. The validate commands can expect the config before and after the update to passed to the CLI commands for detailed validation if needed.
This was requested by linkedin team
There was a problem hiding this comment.
We discussed this with LinkedIn: Zhenggen Xu zxu@linkedin.com; Praveen Chaudhary pchaudhary@linkedin.com on 3/15, from the notes:
Specifically when they delete a port they not only verify it was deleted from ConfigDb, but also from AsicDB. Check code here.
So in order to support their request, we are passing to the validation function what commands were deleted/added. I just made it a little bit generic and passed the whole configDb before and after the update. The validate script itself can do diffing to identify these commands that were changed, or they can loop over the PORTs and check which was deleted.
Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>
qiluo-msft
force-pushed
the
qiluo/fix3
branch
from
d42a594 to
7861c58
Compare
Signed-off-by: Qi Luo <qiluo-msft@users.noreply.github.com>
qiluo-msft
force-pushed
the
qiluo/fix3
branch
from
e383fcc to
727685d
Compare
|
This pull request introduces 4 alerts when merging 727685d into f76f672 - view on LGTM.com new alerts:
|
What I did
Implement ChangeApplier for ConfigDb format
design doc: https://github.com/Azure/SONiC/blob/master/doc/config-generic-update-rollback/Json_Change_Application_Design.md#312-change-applier
How I did it
How to verify it
TODO: manual test on DUT
Previous command output (if the output of a command-line utility has changed)
New command output (if the output of a command-line utility has changed)