Skip to content

Expanding the manifest capabilities #2952

Open
azmy98 wants to merge 2 commits intosonic-net:masterfrom
azmy98:manifest_enhancement
Open

Expanding the manifest capabilities #2952
azmy98 wants to merge 2 commits intosonic-net:masterfrom
azmy98:manifest_enhancement

Conversation

@azmy98
Copy link

@azmy98 azmy98 commented Aug 23, 2023

Expand docker manifest capabilities

What I did

Expanding the manifest.json file to include new network capabilities including the
ports forwarding and docker network type configuration: host, bridge, etc.

How I did it

updating the manifest py file as well as the service_creatore py file.

How to verify it

In order to verify this feature, we need to have the complementary part in the sonic-buildimage repo
and then need to install a docker image file with manifest that includes ports in it and network type as well using sonic-package-manager and validate that they are as expected using docker commands such as: 'docker ps' and 'docker inspect' commands

@azmy98 azmy98 force-pushed the manifest_enhancement branch 2 times, most recently from 68e11a4 to 6ce9955 Compare August 28, 2023 11:05
@azmy98 azmy98 force-pushed the manifest_enhancement branch 2 times, most recently from 26ff973 to 34eea1c Compare August 29, 2023 12:42
@azmy98 azmy98 force-pushed the manifest_enhancement branch 3 times, most recently from 364f639 to ef40341 Compare September 4, 2023 06:37
stepanblyschak
stepanblyschak previously approved these changes Sep 11, 2023
@lguohan
Copy link
Contributor

lguohan commented Sep 23, 2023

this is adding new manifest for 3rd container management, can you update the doc? and provide link to the doc update pr?

@azmy98
Copy link
Author

azmy98 commented Nov 26, 2023

@lguohan, I will add a documentation once the another PR is also approved: sonic-net/sonic-buildimage#16248

@azmy98
Copy link
Author

azmy98 commented Feb 7, 2024

@lguohan, I added the documentation PR: sonic-net/SONiC#1601

@azmy98 azmy98 changed the title Expanding the manifest capabilities to include Expanding the manifest capabilities Jun 19, 2024
run_opt.append(f'--net={docker_network_type}')
else:
if container_spec['network']:
raise ServiceCreatorError(f"Invalid Configuration, asic-service must not contain network type")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not contain network type

empty value means default value. It is not proper to enforce empty value only.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But ASIC services should not have network specified. This comment was given by the NVIDIA SONiC team. I am missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

@azmy98 azmy98 Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you suggest? to enforce the none as the docker_network_type like this
run_opt.append(f'--net=none')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, let me clarify:

  1. container_spec['network'] should have default value bridge. ie. if it is None, treat it as bridge.
  2. I do not propose to append run_opt, always respect user config
  3. if is_asic_service and container_spec['network'] != none, you can raise an exception.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, but this is what I do in this if and else that i added.
for point 3, if the container_spec['network'] is None I raise exception, am I missing something?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting to do explicitly container_spec['network'] != 'none'?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@qiluo-msft
Copy link
Contributor

Please resolve conflict

ports forwarding as well as docker network type
configuration
@azmy98 azmy98 force-pushed the manifest_enhancement branch from ef40341 to 51479cb Compare November 26, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants