Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expanding the manifest capabilities #2952

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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 2 times, most recently from 4fded40 to 364f639 Compare September 3, 2023 11:17
ports forwarding as well as docker network type
configuration
@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

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.

5 participants