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.

Sign up for GitHub

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

Jump to bottom

Expanding the manifest capabilities #2952

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+29 −0
Open

Expanding the manifest capabilities #2952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions sonic_package_manager/manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ def unmarshal(self, value):
ManifestRoot('container', [
ManifestField('privileged', DefaultMarshaller(bool), False),
ManifestArray('volumes', DefaultMarshaller(str)),
ManifestField('network', DefaultMarshaller(str), ''),
azmy98 marked this conversation as resolved.
Show resolved Hide resolved
ManifestArray('ports', DefaultMarshaller(str)),
azmy98 marked this conversation as resolved.
Show resolved Hide resolved
ManifestArray('mounts', ManifestRoot('mounts', [
ManifestField('source', DefaultMarshaller(str)),
ManifestField('target', DefaultMarshaller(str)),
Expand Down
12 changes: 12 additions & 0 deletions sonic_package_manager/service_creator/creator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ def generate_container_mgmt(self, package: Package):
image_id = package.image_id
name = package.manifest['service']['name']
container_spec = package.manifest['container']
is_asic_service = package.manifest['service']['asic-service']
script_path = os.path.join(DOCKER_CTL_SCRIPT_LOCATION, f'{name}.sh')
script_template = get_tmpl_path(DOCKER_CTL_SCRIPT_TEMPLATE)
run_opt = []
Expand All @@ -231,6 +232,17 @@ def generate_container_mgmt(self, package: Package):

run_opt.append('-t')

if not is_asic_service:
azmy98 marked this conversation as resolved.
Show resolved Hide resolved
if container_spec['network']:
docker_network_type = container_spec['network']
run_opt.append(f'--net={docker_network_type}')
else:
if container_spec['network']:
azmy98 marked this conversation as resolved.
Show resolved Hide resolved
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.

Maybe none? ref: https://docs.docker.com/engine/network/

Copy link
Author

@azmy98 azmy98 Sep 10, 2024 •
edited
Loading

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


for port in container_spec['ports']:
run_opt.append(f'--publish {port}')
azmy98 marked this conversation as resolved.
Show resolved Hide resolved

for volume in container_spec['volumes']:
run_opt.append(f'-v {volume}')

Expand Down
15 changes: 15 additions & 0 deletions tests/sonic_package_manager/test_service_creator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ def manifest():
},
'container': {
'privileged': True,
'network': 'bridge',
'ports': [
"8080:8080"
],
'volumes': [
'/etc/sonic:/etc/sonic:ro'
]
Expand Down Expand Up @@ -114,6 +118,17 @@ def read_file(name):
assert read_file('test_reconcile') == 'test-process test-process-3'


def test_service_creator_asic_service_network_type_err(sonic_fs, manifest, service_creator, package_manager):
new_manifest = copy.deepcopy(manifest)
new_manifest['service']['asic-service'] = True
entry = PackageEntry('test', 'azure/sonic-test')
package = Package(entry, Metadata(new_manifest))
installed_packages = package_manager._get_installed_packages_and(package)

with pytest.raises(ServiceCreatorError) as e:
service_creator.create(package)


def test_service_creator_with_timer_unit(sonic_fs, manifest, service_creator):
entry = PackageEntry('test', 'azure/sonic-test')
package = Package(entry, Metadata(manifest))
Expand Down
Loading