Skip to content

Commit

Permalink
[sonic-package-manager] code style fixes and enhancements (sonic-net#…
Browse files Browse the repository at this point in the history
…1802)

Highlights:
* Don't use f-strings where it is not needed
* Indentation fixes in comprehension expressions
* Created containers removal method in DockerApi
* Fixed a debug message where dependency instead of conflict variable
was displayed
* Return value type annotations for some functions
* Docstrings fixes & removed trailing spaces.

Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
  • Loading branch information
stepanblyschak authored Oct 27, 2021
1 parent f53baac commit 776fddf
Show file tree
Hide file tree
Showing 10 changed files with 115 additions and 87 deletions.
2 changes: 1 addition & 1 deletion sonic_package_manager/constraint.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ def parse(constraints: Dict) -> 'ComponentConstraints':
"""

components = {component: VersionConstraint.parse(version)
for component, version in constraints.items()}
for component, version in constraints.items()}
return ComponentConstraints(components)

def deparse(self) -> Dict[str, str]:
Expand Down
9 changes: 9 additions & 0 deletions sonic_package_manager/dockerapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,15 @@ def rm(self, container: str, **kwargs):
self.client.containers.get(container).remove(**kwargs)
log.debug(f'removed container {container}')

def rm_by_ancestor(self, image_id: str, **kwargs):
""" Docker 'rm' command for running containers instantiated
from image passed to this function. """

# Clean containers based on the old image
containers = self.ps(filters={'ancestor': image_id}, all=True)
for container in containers:
self.rm(container.id, **kwargs)

def ps(self, **kwargs):
""" Docker 'ps' command. """

Expand Down
1 change: 0 additions & 1 deletion sonic_package_manager/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,4 +143,3 @@ class PackageComponentConflictError(PackageInstallationError):
def __str__(self):
return (f'Package {self.name} conflicts with {self.component} {self.constraint} '
f'in package {self.dependency} but version {self.installed_ver} is installed')

16 changes: 10 additions & 6 deletions sonic_package_manager/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ def install(ctx,

package_source = package_expr or from_repository or from_tarball
if not package_source:
exit_cli(f'Package source is not specified', fg='red')
exit_cli('Package source is not specified', fg='red')

if not yes and not force:
click.confirm(f'{package_source} is going to be installed, '
Expand All @@ -386,7 +386,7 @@ def install(ctx,
except Exception as err:
exit_cli(f'Failed to install {package_source}: {err}', fg='red')
except KeyboardInterrupt:
exit_cli(f'Operation canceled by user', fg='red')
exit_cli('Operation canceled by user', fg='red')


@cli.command()
Expand All @@ -409,7 +409,7 @@ def reset(ctx, name, force, yes, skip_host_plugins):
except Exception as err:
exit_cli(f'Failed to reset package {name}: {err}', fg='red')
except KeyboardInterrupt:
exit_cli(f'Operation canceled by user', fg='red')
exit_cli('Operation canceled by user', fg='red')


@cli.command()
Expand All @@ -426,12 +426,16 @@ def uninstall(ctx, name, force, yes):
click.confirm(f'Package {name} is going to be uninstalled, '
f'continue?', abort=True, show_default=True)

uninstall_opts = {
'force': force,
}

try:
manager.uninstall(name, force)
manager.uninstall(name, **uninstall_opts)
except Exception as err:
exit_cli(f'Failed to uninstall package {name}: {err}', fg='red')
except KeyboardInterrupt:
exit_cli(f'Operation canceled by user', fg='red')
exit_cli('Operation canceled by user', fg='red')


@cli.command()
Expand All @@ -453,7 +457,7 @@ def migrate(ctx, database, force, yes, dockerd_socket):
except Exception as err:
exit_cli(f'Failed to migrate packages {err}', fg='red')
except KeyboardInterrupt:
exit_cli(f'Operation canceled by user', fg='red')
exit_cli('Operation canceled by user', fg='red')


if __name__ == "__main__":
Expand Down
89 changes: 48 additions & 41 deletions sonic_package_manager/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
from sonic_package_manager.progress import ProgressManager
from sonic_package_manager.reference import PackageReference
from sonic_package_manager.registry import RegistryResolver
from sonic_package_manager.service_creator import SONIC_CLI_COMMANDS
from sonic_package_manager.service_creator.creator import (
ServiceCreator,
run_command
Expand All @@ -52,7 +53,6 @@
RegistrySource,
TarballSource
)
from sonic_package_manager.utils import DockerReference
from sonic_package_manager.version import (
Version,
version_to_tag,
Expand Down Expand Up @@ -101,7 +101,7 @@ def wrapped_function(*args, **kwargs):
return wrapped_function


def rollback(func, *args, **kwargs):
def rollback(func, *args, **kwargs) -> Callable:
""" Used in rollback callbacks to ignore failure
but proceed with rollback. Error will be printed
but not fail the whole procedure of rollback. """
Expand Down Expand Up @@ -131,7 +131,7 @@ def package_constraint_to_reference(constraint: PackageConstraint) -> PackageRef
return PackageReference(package_name, version_to_tag(version))


def parse_reference_expression(expression):
def parse_reference_expression(expression) -> PackageReference:
try:
return package_constraint_to_reference(PackageConstraint.parse(expression))
except ValueError:
Expand Down Expand Up @@ -247,7 +247,7 @@ def validate_package_tree(packages: Dict[str, Package]):
continue

component_version = conflicting_package.components[component]
log.debug(f'conflicting package {dependency.name}: '
log.debug(f'conflicting package {conflict.name}: '
f'component {component} version is {component_version}')

if constraint.allows(component_version):
Expand Down Expand Up @@ -397,12 +397,17 @@ def install_from_source(self,
if not self.database.has_package(package.name):
self.database.add_package(package.name, package.repository)

service_create_opts = {
'state': feature_state,
'owner': default_owner,
}

try:
with contextlib.ExitStack() as exits:
source.install(package)
exits.callback(rollback(source.uninstall, package))

self.service_creator.create(package, state=feature_state, owner=default_owner)
self.service_creator.create(package, **service_create_opts)
exits.callback(rollback(self.service_creator.remove, package))

self.service_creator.generate_shutdown_sequence_files(
Expand Down Expand Up @@ -481,13 +486,7 @@ def uninstall(self, name: str, force=False):
self.service_creator.generate_shutdown_sequence_files(
self._get_installed_packages_except(package)
)

# Clean containers based on this image
containers = self.docker.ps(filters={'ancestor': package.image_id},
all=True)
for container in containers:
self.docker.rm(container.id, force=True)

self.docker.rm_by_ancestor(package.image_id, force=True)
self.docker.rmi(package.image_id, force=True)
package.entry.image_id = None
except Exception as err:
Expand Down Expand Up @@ -563,6 +562,13 @@ def upgrade_from_source(self,

# After all checks are passed we proceed to actual upgrade

service_create_opts = {
'register_feature': False,
}
service_remove_opts = {
'deregister_feature': False,
}

try:
with contextlib.ExitStack() as exits:
self._uninstall_cli_plugins(old_package)
Expand All @@ -576,19 +582,15 @@ def upgrade_from_source(self,
exits.callback(rollback(self._systemctl_action,
old_package, 'start'))

self.service_creator.remove(old_package, deregister_feature=False)
self.service_creator.remove(old_package, **service_remove_opts)
exits.callback(rollback(self.service_creator.create, old_package,
register_feature=False))
**service_create_opts))

# Clean containers based on the old image
containers = self.docker.ps(filters={'ancestor': old_package.image_id},
all=True)
for container in containers:
self.docker.rm(container.id, force=True)
self.docker.rm_by_ancestor(old_package.image_id, force=True)

self.service_creator.create(new_package, register_feature=False)
self.service_creator.create(new_package, **service_create_opts)
exits.callback(rollback(self.service_creator.remove, new_package,
register_feature=False))
**service_remove_opts))

self.service_creator.generate_shutdown_sequence_files(
self._get_installed_packages_and(new_package)
Expand Down Expand Up @@ -654,16 +656,16 @@ def migrate_packages(self,
old_package_database: PackageDatabase,
dockerd_sock: Optional[str] = None):
"""
Migrate packages from old database. This function can do a comparison between
current database and the database passed in as argument. If the package is
missing in the current database it will be added. If the package is installed
in the passed database and in the current it is not installed it will be
installed with a passed database package version. If the package is installed
in the passed database and it is installed in the current database but with
older version the package will be upgraded to the never version. If the package
is installed in the passed database and in the current it is installed but with
never version - no actions are taken. If dockerd_sock parameter is passed, the
migration process will use loaded images from docker library of the currently
Migrate packages from old database. This function can do a comparison between
current database and the database passed in as argument. If the package is
missing in the current database it will be added. If the package is installed
in the passed database and in the current it is not installed it will be
installed with a passed database package version. If the package is installed
in the passed database and it is installed in the current database but with
older version the package will be upgraded to the never version. If the package
is installed in the passed database and in the current it is installed but with
never version - no actions are taken. If dockerd_sock parameter is passed, the
migration process will use loaded images from docker library of the currently
installed image.
Args:
Expand Down Expand Up @@ -784,7 +786,7 @@ def get_package_source(self,
ref = parse_reference_expression(package_expression)
return self.get_package_source(package_ref=ref)
elif repository_reference:
repo_ref = DockerReference.parse(repository_reference)
repo_ref = utils.DockerReference.parse(repository_reference)
repository = repo_ref['name']
reference = repo_ref['tag'] or repo_ref['digest']
reference = reference or 'latest'
Expand Down Expand Up @@ -815,8 +817,8 @@ def get_package_source(self,
if package_entry.default_reference is not None:
package_ref.reference = package_entry.default_reference
else:
raise PackageManagerError(f'No default reference tag. '
f'Please specify the version or tag explicitly')
raise PackageManagerError('No default reference tag. '
'Please specify the version or tag explicitly')

return RegistrySource(package_entry.repository,
package_ref.reference,
Expand Down Expand Up @@ -888,7 +890,7 @@ def get_installed_packages_list(self) -> List[Package]:
Installed packages dictionary.
"""

return [self.get_installed_package(entry.name)
return [self.get_installed_package(entry.name)
for entry in self.database if entry.installed]

def _migrate_package_database(self, old_package_database: PackageDatabase):
Expand Down Expand Up @@ -948,11 +950,11 @@ def _systemctl_action(self, package: Package, action: str):
run_command(f'systemctl {action} {name}@{npu}')

def _install_cli_plugins(self, package: Package):
for command in ('show', 'config', 'clear'):
for command in SONIC_CLI_COMMANDS:
self._install_cli_plugin(package, command)

def _uninstall_cli_plugins(self, package: Package):
for command in ('show', 'config', 'clear'):
for command in SONIC_CLI_COMMANDS:
self._uninstall_cli_plugin(package, command)

def _install_cli_plugin(self, package: Package, command: str):
Expand All @@ -978,12 +980,17 @@ def get_manager() -> 'PackageManager':
PackageManager
"""

docker_api = DockerApi(docker.from_env())
docker_api = DockerApi(docker.from_env(), ProgressManager())
registry_resolver = RegistryResolver()
return PackageManager(DockerApi(docker.from_env(), ProgressManager()),
metadata_resolver = MetadataResolver(docker_api, registry_resolver)
feature_registry = FeatureRegistry(SonicDB)
service_creator = ServiceCreator(feature_registry,
SonicDB)

return PackageManager(docker_api,
registry_resolver,
PackageDatabase.from_file(),
MetadataResolver(docker_api, registry_resolver),
ServiceCreator(FeatureRegistry(SonicDB), SonicDB),
metadata_resolver,
service_creator,
device_info,
filelock.FileLock(PACKAGE_MANAGER_LOCK_FILE, timeout=0))
7 changes: 4 additions & 3 deletions sonic_package_manager/metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ def deep_update(dst: Dict, src: Dict) -> Dict:

for key, value in src.items():
if isinstance(value, dict):
node = dst.setdefault(key, {})
deep_update(node, value)
node = dst.setdefault(key, {})
deep_update(node, value)
else:
dst[key] = value
dst[key] = value
return dst


Expand Down Expand Up @@ -183,3 +183,4 @@ def from_labels(cls, labels: Dict[str, str]) -> Metadata:
raise MetadataError(f'Failed to parse component version: {err}')

return Metadata(Manifest.marshal(manifest_dict), components)

2 changes: 1 addition & 1 deletion sonic_package_manager/registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def get_token(realm, service, scope) -> str:

response = requests.get(f'{realm}?scope={scope}&service={service}')
if response.status_code != requests.codes.ok:
raise AuthenticationServiceError(f'Failed to retrieve token')
raise AuthenticationServiceError('Failed to retrieve token')

content = json.loads(response.content)
token = content['token']
Expand Down
1 change: 1 addition & 0 deletions sonic_package_manager/service_creator/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#!/usr/bin/env python

ETC_SONIC_PATH = '/etc/sonic'
SONIC_CLI_COMMANDS = ('show', 'config', 'clear')
Loading

0 comments on commit 776fddf

Please sign in to comment.