ThirdPartyContainerManagement(TPCM)_in_SonicPackageManager #2815

sg893052 · 2023-05-02T10:34:39Z

HLD : https://github.com/sonic-net/SONiC/blob/master/doc/sonic-application-extension/tpcm_app_ext.md

ThirdPartyContainerManagement(TPCM) support in SonicPackageManager allows third party dockers to be installed on the sonic system. The Manifest file is generated from a custom local default file. The Manifest file could be updated through "sonic-package-manager manifests update" command and later the running package could be updated with the new manifest file through "sonic-package-manager update"

What I did

There are many Third Party application dockers, that can be used in SONiC to provision, manage and monitor SONiC devices. The dockers need not be compatible with SONiC, but can almost work independently with minimal SONiC interfaces. These are extensions to SONiC and require additional capabilities to seamlessly integrate with SONiC. These are related to installation, upgrade, and configuration. This change is an enhancement to the SONiC Application Extension Infrastructure to enable integrating a Third Party Application in the form of dockers with SONiC.
Moreover, the process of downloading image tarballs for the dockers (packages) supports SCP, SFTP, and URL before installing them.

How I did it

The Sonic-package-manager framework has been enhanced to support ThirdPartyContainerManagement (TPCM). In case no manifest is found in the image labels, the framework treats it as a TPCM package and creates a default manifest for it. During installation, a new manifest file is created with a specified name using the --name option. Users can use the "sonic-package-manager manifests create/update/delete" commands to modify or delete the manifest file. The location for custom local package manifest files is set to "/var/lib/sonic-package-manager/manifests/". Finally, the "sonic-package-manager update" command can be used to apply the updated manifest file to the running TPCM docker.

How to verify it

sonic-package-manager install --from-repository <package without manifest, say httpd> --name mytpcm

sonic-package manager install --from-tarball <local tar/scp tar/sftp tar/http tar> --name <> --use-local-manifest

Manifests Commands(tpcm):
sonic-package-manager manifests create <> --from-json <>
sonic-package-manager manifests update <> --from-json <>
sonic-package-manager manifests list
sonic-package-manager manifests show <>
sonic-package-manager manifests delete <>

sonic-package manager update

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)

adyeung · 2024-01-12T01:11:32Z

@venkatmahalingam @stepanblyschak pls review the Code PR

stepanblyschak · 2024-01-19T11:53:39Z

sonic_package_manager/main.py

+    if manager.is_installed(name):
+        click.echo("Error: A package with the same name {} is already installed".format(name))
+        return
+    MFILE_NAME = os.path.join(MANIFEST_LOCATION, name)


MFILE_NAME -> mfile_name since not a global constant

stepanblyschak · 2024-01-19T11:53:59Z

sonic_package_manager/main.py

+    if not os.path.exists(DMFILE_NAME):
+        with open(DMFILE_NAME, 'w') as file:
+            json.dump(DEFAULT_MANIFEST, file, indent=4)
+        #click.echo(f"Manifest '{DEFAUT_MANIFEST_NAME}' created now.")


Remove comment

stepanblyschak · 2024-01-19T11:54:47Z

sonic_package_manager/main.py

+@click.pass_context
+@click.argument('name', type=click.Path())
+@click.option('--from-json', type=str, required=True)
+#@click.argument('--from-json', type=str, help='Specify Manifest json file')


Remove comment

sonic_package_manager/main.py

stepanblyschak · 2024-01-19T11:55:28Z

sonic_package_manager/main.py

+    """Update an existing custom local manifest file with new one."""
+
+    manager: PackageManager = ctx.obj
+    ORG_FILE = os.path.join(MANIFEST_LOCATION, name)


Does org mean "original"? if so I prefer longer variable name

stepanblyschak · 2024-01-20T17:17:49Z

tests/sonic_package_manager/test_metadata.py

+def test_get_manifest_from_local_file2(capsys):
+    metadata_resolver = MetadataResolver(None, None)  # Replace None with appropriate mocks
+
+    with patch('os.path.exists', return_value=True), \


This isn't needed. To simplify mocking FS operations there is a sonic_fs fixture based on pyfakefs

stepanblyschak · 2024-01-20T17:27:49Z

sonic_package_manager/main.py

+
+    #Validation checks
+    manager: PackageManager = ctx.obj
+    if manager.is_installed(name):


What if manifest is created before the package with the same name is installed?

Changed the logic such a way that, it first checks if a manifests with name "name" exists and use it. Else creates a new manifest with the name "name" populated from default manifest.

stepanblyschak · 2024-01-20T17:33:38Z

sonic_package_manager/metadata.py

        if labels is None:
-            raise MetadataError('No manifest found in image labels')
-
+                    raise MetadataError('No manifest found in image labels and also could not create locally')    


stepanblyschak · 2024-01-20T17:40:31Z

sonic_package_manager/source.py

@@ -105,20 +105,22 @@ def __init__(self,
                 tarball_path: str,
                 database: PackageDatabase,
                 docker: DockerApi,
-                 metadata_resolver: MetadataResolver):
+                 metadata_resolver: MetadataResolver,
+                 use_local_manifest: bool = False,


There seems to be no point in passing in use_local_manifest through all the objects.

The logic is - if manifest with the name name exists use it unless docker has one, otherwise use what's inside the docker. This also means you don't need to modify MetadataResolver as it is relevant only for the second case.

The logic is now maintained as suggested.
To first use the manifest inside docker image. If that does not exist and If the custom local manifest with name "name" exists, use it.
The option to prioritize the local manifest over the docker image built-in manifest is hidden.

stepanblyschak · 2024-01-20T17:44:21Z

sonic_package_manager/main.py

@@ -378,17 +642,50 @@ def install(ctx,
    if allow_downgrade is not None:
        install_opts['allow_downgrade'] = allow_downgrade

+    if use_local_manifest:


Please disallow the use of custom user manifest on dockers that provide their own. This might create a lot of irrelevant complains from customers trying to do that and breaking the package functionality. Only allow to use custom manifest on dockers that don't have them.

For now, I have made this option(use_local_manifest) to be hidden. Have kept this code as such in case if there is a requirement in future to override the built-in manifest.

sg893052 · 2024-04-01T10:10:18Z

sonic_package_manager/manager.py

@@ -838,10 +813,10 @@ def migrate_package(old_package_entry,
                package_source = self.get_package_source(package_ref=new_package_ref)
                package = package_source.get_package()
                new_package_default_version = package.manifest['package']['version']
-                if old_package.version > new_package_default_version:
+                if old_package.version >= new_package_default_version:


For instance, Third party container debian:latest with name MYDEB was downloaded to the switch( default_version will be 1.0.0 and default_reference will be latest)
Now, on attempt to migrate, the new_package_default_reference will also be 1.0.0.
In this case as well, we could migrate the MYDEB tpcm docker using image save technique of migrate_package.

@sg893052 If I understand correctly, package is migrated even if the it is of the same version in the new image?
If you installed a package MYDEB in the old life, why it should be present in the new life and have the same version, even then, why we should migrate?

@stepanblyschak Please suggest if the below is OK.

MYDEB package (debian:latest) is installed in the current image.
Now, on sonic-sonic image upgrade, as part of new image, MYDEB package is not installed.
Hence as part of migrate_packages(), "elif new_package.default_reference is not None:" is hit and finally the version comparison occurs where the old_package.version [1.0.0] and new_package_default_version are same [1.0.0].
As per the original code, calling self.install("MYDEB=1.0.0") in the else case would fail as below

Failed to migrate packages Failed to retrieve manifest for library/debian:1.0.0: code: 404 details: { 'errors': [ { 'code': 'MANIFEST_UNKNOWN', 'message': 'manifest unknown', 'detail': 'unknown tag=1.0.0' } ] }

For this to work, we could change the else case code to below (if migration is not supposed to be called in version equal case)

else: #self.install(f'{new_package.name}={new_package_default_version}') repo_tag_formed="{}:{}".format(new_package.repository, new_package.default_reference) self.install(None,repo_tag_formed,name=new_package.name)

@sg893052 Could you please check why you have default_reference set to 1.0.0? In your case I would expect the following will execute:

else: # No default version and package is not installed. # Migrate old package same version. new_package.version = old_package.version migrate_package(old_package, new_package)

The default_reference is a tag or digest of Docker image that will be a default installation candidate. 1.0.0 does not exists for debian docker image, therefore you get this error.

@stepanblyschak In the migrate_package function, the following condition cases are defined, but there is still some confusion that requires clarification:

Case 1: if new_package.installed.
Case 2: else if new_package.default_reference is not None.
Case 3: else.

In the specific example of the MYDEB package (debian:latest), where "latest" is set as the new_package.default_reference, Case 2 will be triggered because new_package.default_reference is not None. However, Case 3 will not be triggered in this scenario.

@sg893052 The error message you shared before implies you had '1.0.0' as the default_reference "Failed to migrate packages Failed to retrieve manifest for library/debian:1.0.0:". What is the problem if default_reference is set to 'latest'?

@stepanblyschak Firstly, new_package.default_reference is "latest" which makes it to hit the case2 .

Subsequently, new_package_default_version = package.manifest['package']['version'] --> new_package_default_version is derived from package's manifest package version which is 1.0.0 .
Hence the new_package_default_version variable is set to 1.0.0 here

Log:

package: Package(entry=PackageEntry(name='MYDEB', repository='debian', description='', default_reference='latest', version=None, installed=False, built_in=False, image_id=None), metadata=Metadata(manifest={'version': Version('1.0.0'), 'package': {'version': Version('1.0.0'), 'name': 'MYDEB', 'description': '', 'base-os': ComponentConstraints(components={}), 'depends': [], 'breaks': [], 'init-cfg': {}, 'changelog': {}, 'debug-dump': ''}, 'service': {'name': 'MYDEB', 'requires': [], 'requisite': [], 'wanted-by': [], 'after': [], 'before': [], 'dependent': [], 'dependent-of': [], 'post-start-action': '', 'pre-shutdown-action': '', 'asic-service': False, 'host-service': True, 'delayed': False, 'check_up_status': False, 'warm-shutdown': {'after': [], 'before': []}, 'fast-shutdown': {'after': [], 'before': []}, 'syslog': {'support-rate-limit': False}}, 'container': {'privileged': False, 'volumes': [], 'mounts': [], 'environment': {}, 'tmpfs': []}, 'processes': [], 'cli': {'mandatory': False, 'show': [], 'config': [], 'clear': [], 'auto-generate-show': False, 'auto-generate-config': False, 'auto-generate-show-source-yang-modules': [], 'auto-generate-config-source-yang-modules': []}}, components={}, yang_modules=[]))

migrate -- old_package.version:1.0.0 new_package_default_version: 1.0.0

==================================================================

Code snippet:

elif new_package.default_reference is not None: new_package_ref = PackageReference(new_package.name, new_package.default_reference) package_source = self.get_package_source(package_ref=new_package_ref) package = package_source.get_package() print("package:{}".format(package)) new_package_default_version = package.manifest['package']['version'] print("migrate -- old_package.version:{} new_package_default_version: {}".format(old_package.version, new_package_default_version)) if old_package.version > new_package_default_version:

@sg893052 Thanks for all the details.

I agree with your following suggestion:

For this to work, we could change the else case code to below (if migration is not supposed to be called in version equal case)

else: #self.install(f'{new_package.name}={new_package_default_version}') repo_tag_formed="{}:{}".format(new_package.repository, new_package.default_reference) self.install(None,repo_tag_formed,name=new_package.name)

If the logic found new_package.default_reference is not None and decided to install from default_reference it should use default_reference instead of version. The code currently assumes default_reference is the same as default_version, however it's not always the case.

Thanks! Incorporated it in latest commit

stepanblyschak · 2024-04-07T09:44:13Z

setup.py

@@ -257,6 +257,7 @@
        'xmltodict==0.12.0',
        'lazy-object-proxy',
        'six==1.16.0',
+        'scp',


Pin this package to a certain version tested with like other dependencies are.

stepanblyschak · 2024-04-07T09:49:34Z

sonic_package_manager/main.py

+        if not name:
+            click.echo(f'name argument is not provided to use local manifest')
+            return
+        ORG_FILE = os.path.join(MANIFEST_LOCATION, name)


Rename to lower case to indicate this is not a global constant

stepanblyschak · 2024-04-07T09:52:01Z

sonic_package_manager/manager.py

@@ -430,7 +433,7 @@ def install_from_source(self,
                    self.service_creator.generate_shutdown_sequence_files,
                    self.get_installed_packages())
                )
-
+                


@sg893052 Could you please check again, I see spaces in the diff

stepanblyschak · 2024-04-07T09:56:19Z

sonic_package_manager/manager.py

@@ -838,10 +813,10 @@ def migrate_package(old_package_entry,
                package_source = self.get_package_source(package_ref=new_package_ref)
                package = package_source.get_package()
                new_package_default_version = package.manifest['package']['version']
-                if old_package.version > new_package_default_version:
+                if old_package.version >= new_package_default_version:


@sg893052 If I understand correctly, package is migrated even if the it is of the same version in the new image?
If you installed a package MYDEB in the old life, why it should be present in the new life and have the same version, even then, why we should migrate?

stepanblyschak · 2024-04-07T09:59:30Z

sonic_package_manager/manifest.py

+}
+#DEFAULT_MANIFEST = {'version': '1.0.0', 'package': {'version': '1.0.0', 'depends': [], 'name': 'default_manifest'}, 'service': {'name': 'default_manifest', 'requires': ['docker'], 'after': ['docker'], 'before': [], 'dependent-of': [], 'asic-service': False, 'host-service': False, 'warm-shutdown': {'after': [], 'before': []}, 'fast-shutdown':
+    #{'after': [], 'before': []}, 'syslog': {'support-rate-limit': False}}, 'container': {'privileged': False, 'volumes': [], 'tmpfs': [], 'entrypoint': ''}, 'cli': { 'mandatory': False, 'config': [], 'show': [], 'clear': []}}
+MANIFEST_LOCATION = "/var/lib/sonic-package-manager/manifests/"


@sg893052 Could you please double check if the rename was commited?

sg893052 · 2024-04-22T03:41:35Z

setup.py

@@ -257,6 +257,7 @@
        'xmltodict==0.12.0',
        'lazy-object-proxy',
        'six==1.16.0',
+        'scp==0.14.5',


Fixed the version number as per suggestion

sg893052 · 2024-04-22T03:54:21Z

sonic_package_manager/manifest.py

+        "auto-generate-config-source-yang-modules": []
+    }
+}
+MANIFESTS_LOCATION = "/var/lib/sonic-package-manager/manifests/"


updated the variable from MANIFEST_LOCATION to MANIFESTS_LOCATION

adyeung · 2024-04-26T00:07:50Z

@stepanblyschak please help review the changes from Senthil again, this is a planned feature for 202405

stepanblyschak

One open question regarding changes in the package migration logic

stepanblyschak · 2024-05-03T08:37:32Z

sonic_package_manager/manifest.py

+                    'sonic': {
+                        'manifest': json_str
+                            }


Please fix formatting

Taken care of in latest commit

stepanblyschak · 2024-05-03T08:39:54Z

sonic_package_manager/manifest.py

+        if not os.path.exists(MANIFESTS_LOCATION):
+            os.mkdir(MANIFESTS_LOCATION)
+        if not os.path.exists(DEFAULT_MANIFEST_FILE):
+            with open(DEFAULT_MANIFEST_FILE, 'w') as file:
+                json.dump(DEFAULT_MANIFEST, file, indent=4)


This seems to duplicate the code from create_package_manifest, why we need to create a directory and default manifest file here in get_manifest_from_local_file ?

create_package_manifest is invoked as part of "sonic_package_manager manifests create" command.
If the user does not invoke this CLI first but rather directly executes "sonic_package_manager install" command, then these directory & default manifest file are to be created on the fly for the first time.

@sg893052 It seems like you can prepare the directory and the default manifest at build time without the need to create it in runtime every time it is required

For this, creation of directory and default manifest is taken care in sonic-buidimage repo similar to packages.json file.
https://github.com/sonic-net/sonic-buildimage/pull/14917/files

stepanblyschak · 2024-05-03T09:06:46Z

sonic_package_manager/manager.py

@@ -838,10 +813,10 @@ def migrate_package(old_package_entry,
                package_source = self.get_package_source(package_ref=new_package_ref)
                package = package_source.get_package()
                new_package_default_version = package.manifest['package']['version']
-                if old_package.version > new_package_default_version:
+                if old_package.version >= new_package_default_version:


@sg893052 Could you please check why you have default_reference set to 1.0.0? In your case I would expect the following will execute:

else: # No default version and package is not installed. # Migrate old package same version. new_package.version = old_package.version migrate_package(old_package, new_package)

The default_reference is a tag or digest of Docker image that will be a default installation candidate. 1.0.0 does not exists for debian docker image, therefore you get this error.

ThirdPartyContainerManagement(TPCM) support in SonicPackageManager allows third party dockers to be installed on the sonic system. The Manifest file is generated from a local default file. The Manifest file could be updated through "sonic-package-manager manifests update" command and later the running package could be updated with the new manifest file through "sonic-package-manager update"

sg893052 · 2024-05-10T18:29:48Z

@adyeung Reviewer @stepanblyschak has approved the changes.

adyeung · 2024-05-10T18:35:52Z

@qiluo-msft please help review and merge, this is needed for 202405

…#2815) ThirdPartyContainerManagement(TPCM) support in SonicPackageManager allows third party dockers to be installed on the sonic system. The Manifest file is generated from a custom local default file. The Manifest file could be updated through "sonic-package-manager manifests update" command and later the running package could be updated with the new manifest file through "sonic-package-manager update" #### What I did There are many Third Party application dockers, that can be used in SONiC to provision, manage and monitor SONiC devices. The dockers need not be compatible with SONiC, but can almost work independently with minimal SONiC interfaces. These are extensions to SONiC and require additional capabilities to seamlessly integrate with SONiC. These are related to installation, upgrade, and configuration. This change is an enhancement to the SONiC Application Extension Infrastructure to enable integrating a Third Party Application in the form of dockers with SONiC. Moreover, the process of downloading image tarballs for the dockers (packages) supports SCP, SFTP, and URL before installing them. #### How I did it The Sonic-package-manager framework has been enhanced to support ThirdPartyContainerManagement (TPCM). In case no manifest is found in the image labels, the framework treats it as a TPCM package and creates a default manifest for it. During installation, a new manifest file is created with a specified name using the --name option. Users can use the "sonic-package-manager manifests create/update/delete" commands to modify or delete the manifest file. The location for custom local package manifest files is set to "/var/lib/sonic-package-manager/manifests/". Finally, the "sonic-package-manager update" command can be used to apply the updated manifest file to the running TPCM docker. #### How to verify it sonic-package-manager install --from-repository <package without manifest, say httpd> --name mytpcm sonic-package manager install --from-tarball <local tar/scp tar/sftp tar/http tar> --name <> --use-local-manifest Manifests Commands(tpcm): sonic-package-manager manifests create <> --from-json <> sonic-package-manager manifests update <> --from-json <> sonic-package-manager manifests list sonic-package-manager manifests show <> sonic-package-manager manifests delete <> sonic-package manager update <package>

sg893052 mentioned this pull request May 4, 2023

Third party container management using the Sonic Application Framework sonic-net/SONiC#1286

Merged

sg893052 force-pushed the TPCM branch 5 times, most recently from 2560a9c to cf60307 Compare May 7, 2023 05:36

sg893052 force-pushed the TPCM branch from cf60307 to a8405ca Compare November 15, 2023 15:27

sg893052 force-pushed the TPCM branch 4 times, most recently from ebf186a to 5247f6b Compare November 27, 2023 06:51

stepanblyschak suggested changes Jan 20, 2024

View reviewed changes

sg893052 force-pushed the TPCM branch from 5247f6b to ae2381a Compare March 31, 2024 09:38

sg893052 commented Apr 1, 2024

View reviewed changes

sg893052 force-pushed the TPCM branch 3 times, most recently from f6dc75d to c6457a4 Compare April 4, 2024 13:50

stepanblyschak reviewed Apr 7, 2024

View reviewed changes

sg893052 force-pushed the TPCM branch 2 times, most recently from e9db7a1 to 37cb008 Compare April 21, 2024 17:17

sg893052 commented Apr 22, 2024

View reviewed changes

stepanblyschak reviewed May 3, 2024

View reviewed changes

sg893052 added 2 commits May 9, 2024 11:58

Thrid Party Container Management using Sonic Package Manager - followup1

f5b85dd

sg893052 force-pushed the TPCM branch from 6ef3469 to 5f7f304 Compare May 9, 2024 18:58

Thrid Party Container Management using Sonic Package Manager - followup2

fffeda1

sg893052 force-pushed the TPCM branch from 5f7f304 to fffeda1 Compare May 10, 2024 05:31

stepanblyschak approved these changes May 10, 2024

View reviewed changes

sg893052 force-pushed the TPCM branch 2 times, most recently from c66679c to 73250ca Compare May 10, 2024 16:53

Fix Pretest Static Analysis found formats

c141665

sg893052 force-pushed the TPCM branch from 73250ca to c141665 Compare May 10, 2024 17:43

adyeung approved these changes May 10, 2024

View reviewed changes

qiluo-msft approved these changes May 15, 2024

View reviewed changes

qiluo-msft merged commit 550816b into sonic-net:master May 15, 2024
7 checks passed

ThirdPartyContainerManagement(TPCM)_in_SonicPackageManager #2815

ThirdPartyContainerManagement(TPCM)_in_SonicPackageManager #2815

Conversation

sg893052 commented May 2, 2023 • edited Loading

What I did

How I did it

How to verify it

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)

adyeung commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stepanblyschak Jan 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sg893052 Apr 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sg893052 May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Log:

Choose a reason for hiding this comment

sg893052 May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adyeung commented Apr 26, 2024

stepanblyschak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sg893052 May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sg893052 May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sg893052 commented May 10, 2024

adyeung commented May 10, 2024

sg893052 commented May 2, 2023 •

edited

Loading

stepanblyschak Jan 20, 2024 •

edited

Loading

sg893052 Apr 14, 2024 •

edited

Loading

sg893052 May 7, 2024 •

edited

Loading

sg893052 May 9, 2024 •

edited

Loading

sg893052 May 9, 2024 •

edited

Loading

sg893052 May 6, 2024 •

edited

Loading