[config][generic-update] Adding apply-patch, rollback, checkpoints commands #1536
Conversation
|
This pull request introduces 1 alert when merging 89b161bf2835c8247f2124850cc29a394f8c2e61 into 6b51bcd - view on LGTM.com new alerts:
|
config/generic_update.py
Outdated
| import jsonpatch | ||
| import sonic_yang | ||
| import os | ||
| load_source('sonic_cfggen', '/usr/local/bin/sonic-cfggen') |
There was a problem hiding this comment.
/usr/local/bin/sonic-cfggen [](start = 29, length = 27)
Is it possible not to assume the installation path? I know it may be tricky to import a script.
There was a problem hiding this comment.
Got rid of load_source altogether.
config/generic_update.py
Outdated
| try: | ||
| sy.validate_data_tree() | ||
| return True | ||
| except Exception as ex: |
There was a problem hiding this comment.
Exception [](start = 15, length = 9)
Do not catch general exception type unless you have a good reason.
There was a problem hiding this comment.
I am catching general exception for now, will fix that in the last pr where i fixing logging.
There was a problem hiding this comment.
Code updated to catch specific exceptions,
config/generic_update.py
Outdated
| target_config = __simulate_patch(patch, old_config) | ||
|
|
||
| if not(__validate(target_config)): | ||
| raise Exception(f"The given patch is not valid") |
There was a problem hiding this comment.
Exception [](start = 18, length = 9)
Not to throw general exception type. This looks a normal logic, not like a literal 'exception'. How about using return value?
There was a problem hiding this comment.
throwing general exception for now, will fix that in the logging pr
There was a problem hiding this comment.
throwing custom exception GenericConfigUpdaterError
config/generic_update.py
Outdated
| try: | ||
| sy.validate_data_tree() | ||
| return True | ||
| except Exception as ex: |
There was a problem hiding this comment.
I am catching general exception for now, will fix that in the last pr where i fixing logging.
config/generic_update.py
Outdated
| target_config = __simulate_patch(patch, old_config) | ||
|
|
||
| if not(__validate(target_config)): | ||
| raise Exception(f"The given patch is not valid") |
There was a problem hiding this comment.
throwing general exception for now, will fix that in the logging pr
|
This pull request introduces 5 alerts when merging f8dbdf84e4e606eb957eb15964dc542856986597 into 305a3e4 - view on LGTM.com new alerts:
|
|
This pull request introduces 3 alerts when merging 231c3f4afc3514fc2b9588ad1091773cabadee94 into 305a3e4 - view on LGTM.com new alerts:
|
| from enum import Enum | ||
| from swsssdk import ConfigDBConnector | ||
| from imp import load_source | ||
| load_source('sonic_cfggen', '/usr/local/bin/sonic-cfggen') |
There was a problem hiding this comment.
Was not able to include in install_requires: tried sonic_cfggen and sonic-cfggen.
Also when checking the code behind show runningconfiguration all it is implemeted as:
@runningconfiguration.command()
@click.option('--verbose', is_flag=True, help="Enable verbose output")
def all(verbose):
"""Show full running configuration"""
cmd = "sonic-cfggen -d --print-data"
run_command(cmd, display_cmd=verbose)Which means they just depend on the system to have, and we here are doing the same.
We can either leave it as as, or just rely on the command from cli
There was a problem hiding this comment.
The name is sonic-config-engine. Currently it is tests_require. We should remove it, and add as install_requires.
ghooo
force-pushed
the
dev/mghoneim/generic_update_rollback
branch
2 times, most recently
from
614e56e to
2d3f005
Compare
|
This pull request introduces 2 alerts when merging 2d3f00508455bffb88aff90efef906eee3eda32a into e296a69 - view on LGTM.com new alerts:
|
ghooo
force-pushed
the
dev/mghoneim/generic_update_rollback
branch
4 times, most recently
from
36604f2 to
5d78b22
Compare
ghooo
force-pushed
the
dev/mghoneim/generic_update_rollback
branch
from
5d78b22 to
e5d142c
Compare
|
/azp run |
|
Commenter does not have sufficient privileges for PR 1536 in repo Azure/sonic-utilities |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -4,4 +4,8 @@ ARG docker_container_name | |||
|
|
|||
| ADD ["wheels", "/wheels"] | |||
|
|
|||
| RUN pip3 install --no-deps --force-reinstall /wheels/sonic_utilities-1.2-py3-none-any.whl | |||
| # Uninstalls only sonic-utilities and does not impact its dependencies | |||
There was a problem hiding this comment.
docker-sonic-vs is downloaded then the new sonic-utilities package is installed with most recent modifications. Problem happens if the new package has new external libs or one of the libs is updated as using the command pip3 install with --deps just leave the deps unchanged.
I tried different ideas:
pip3 install /wheels/sonic_utilities-1.2-py3-none-any.whl
does not do anything because the package is already installed in the docker-sonic-vs
pip3 install --force-reinstall /wheels/sonic_utilities-1.2-py3-none-any.whl
does not work because it also forces the deps to be updated but some of the deps such as sonic-py-common are not available in public pip repo and are installed with the sonic image in docker-sonic-vs
pip3 install --update /wheels/sonic_utilities-1.2-py3-none-any.whl
does not update sonic-utilities package and only updates its dependencies
The proposed soln achieved the following:
- Force install sonic-utilities package
- Adding missing dependencies
- Upgrading out-dated dependencies
- Leaving dependencies provided by the sonic-build process unchanged.
|
No pipelines are associated with this pull request. |
1 similar comment
|
No pipelines are associated with this pull request. |
config/main.py
Outdated
| @@ -986,6 +988,129 @@ def load(filename, yes): | |||
| log.log_info("'load' executing...") | |||
| clicommon.run_command(command, display_cmd=True) | |||
|
|
|||
| @config.command('apply-patch') | |||
| @click.argument('patch-file-path', type=str, required=True) | |||
| @click.option('-f', '--format', type=click.Choice([e.name.lower() for e in ConfigFormat]), | |||
There was a problem hiding this comment.
This option is added to make the code ready to use SonicYang format in the future once our internal services start interacting with SONiC boxes using SonicYang. Do you suggest we add it to the design document, remove it from here, or leave it as is?
There was a problem hiding this comment.
Better to update the design doc.
config/main.py
Outdated
| @config.command('apply-patch') | ||
| @click.argument('patch-file-path', type=str, required=True) | ||
| @click.option('-f', '--format', type=click.Choice([e.name.lower() for e in ConfigFormat]), | ||
| default=ConfigFormat.CONFIGDB.name.lower(), |
config/main.py
Outdated
|
|
||
| click.secho("Patch applied successfully.", fg="cyan", underline=True) | ||
| except Exception as ex: | ||
| click.secho("Failed to apply patch", fg="red", underline=True) |
…mmands (sonic-net#1536) #### What I did Adding apply-patch, rollback, replace, checkpoint, delete-checkpoint, list-checkpoints functionality. #### How I did it This PR is implementing the first step in in README.md in the design document: sonic-net/SONiC#736 #### How to verify it Using unit-tests #### 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) ```sh admin@sonic:~$ sudo config apply-patch --help Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH Apply given patch of updates to Config. A patch is a JsonPatch which follows rfc6902. This command can be used do partial updates to the config with minimum disruption to running processes. It allows addition as well as deletion of configs. The patch file represents a diff of ConfigDb(ABNF) format or SonicYang format. <patch-file-path>: Path to the patch file on the file-system. Options: -f, --format [CONFIGDB|SONICYANG] format of config of the patch is either ConfigDb(ABNF) or SonicYang -d, --dry-run test out the command without affecting config state -v, --verbose print additional details of what the operation is doing -h, -?, --help Show this message and exit. admin@sonic:~$ sudo config replace --help Usage: config replace [OPTIONS] TARGET_FILE_PATH Replace the whole config with the specified config. The config is replaced with minimum disruption e.g. if ACL config is different between current and target config only ACL config is updated, and other config/services such as DHCP will not be affected. **WARNING** The target config file should be the whole config, not just the part intended to be updated. <target-file-path>: Path to the target file on the file-system. Options: -f, --format [CONFIGDB|SONICYANG] format of target config is either ConfigDb(ABNF) or SonicYang -d, --dry-run test out the command without affecting config state -v, --verbose print additional details of what the operation is doing -h, -?, --help Show this message and exit. admin@sonic:~$ sudo config rollback --help Usage: config rollback [OPTIONS] CHECKPOINT_NAME Rollback the whole config to the specified checkpoint. The config is rolled back with minimum disruption e.g. if ACL config is different between current and checkpoint config only ACL config is updated, and other config/services such as DHCP will not be affected. <checkpoint-name>: The checkpoint name, use `config list-checkpoints` command to see available checkpoints. Options: -d, --dry-run test out the command without affecting config state -v, --verbose print additional details of what the operation is doing -?, -h, --help Show this message and exit. admin@sonic:~$ sudo config checkpoint --help Usage: config checkpoint [OPTIONS] CHECKPOINT_NAME Take a checkpoint of the whole current config with the specified checkpoint name. <checkpoint-name>: The checkpoint name, use `config list-checkpoints` command to see available checkpoints. Options: -v, --verbose print additional details of what the operation is doing -h, -?, --help Show this message and exit. admin@sonic:~$ sudo config delete-checkpoint --help Usage: config delete-checkpoint [OPTIONS] CHECKPOINT_NAME Delete a checkpoint with the specified checkpoint name. <checkpoint-name>: The checkpoint name, use `config list-checkpoints` command to see available checkpoints. Options: -v, --verbose print additional details of what the operation is doing -h, -?, --help Show this message and exit. admin@sonic:~$ sudo config list-checkpoints --help Usage: config list-checkpoints [OPTIONS] List the config checkpoints available. Options: -v, --verbose print additional details of what the operation is doing -?, -h, --help Show this message and exit. ```
What I did
Adding apply-patch, rollback, replace, checkpoint, delete-checkpoint, list-checkpoints functionality.
How I did it
This PR is implementing the first step in in README.md in the design document: sonic-net/SONiC#736
How to verify it
Using unit-tests
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)