Skip to content

Adjusting docker creation to include ports forwarding and docker network type#16248

Open
azmy98 wants to merge 1 commit intosonic-net:masterfrom
azmy98:sonic_docker_image_ctl_update
Open

Adjusting docker creation to include ports forwarding and docker network type#16248
azmy98 wants to merge 1 commit intosonic-net:masterfrom
azmy98:sonic_docker_image_ctl_update

Conversation

@azmy98
Copy link
Copy Markdown

@azmy98 azmy98 commented Aug 23, 2023

Why I did it

To support docker network configuration and support port forwarding from values in the manifest file

How I did it

update the docker_image_ctl.j2 file

How to verify it

In order to verify this feature, we need to have the complementary part in the sonic-utilities 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 requested a review from lguohan as a code owner August 23, 2023 09:13
@lguohan
Copy link
Copy Markdown
Collaborator

lguohan commented Sep 2, 2023

what feature requires this, can you explain the motivation?

stepanblyschak
stepanblyschak previously approved these changes Sep 11, 2023
@azmy98
Copy link
Copy Markdown
Author

azmy98 commented Sep 19, 2023

@lguohan

what feature requires this, can you explain the motivation?

the purpose of this change is to be able to support docker creation with specified network type (host, bridge) and the forwarding ports found in the docker manifest when installing docker image from sonic-package-manager. Please check the following PR: sonic-net/sonic-utilities#2952 to understand the whole picture. the manifest attributes will finally be exported to dictionary in docker_image_run_opt including the net type of the docker, so it will help us containerize the docker image and block it from accessing all the host network.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if

This if will possibly override NET related feature in this file. Please unify the solution like docker_image_run_opt and stop using NET.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

If a docker does not have network attribute, he will have a default value from this file. I used the if statement to make code backward compatible.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@qiluo-msft, can you please confirm?

@azmy98
Copy link
Copy Markdown
Author

azmy98 commented Nov 27, 2023

@qiluo-msft, there is a conflict also on this PR:

docker create {{docker_image_run_opt}} 
{%- if docker_container_name != "dhcp_server" %}
        --net=$NET \
{%- endif %}

can you please direct me to who wrote this new code:

{%- if docker_container_name != "dhcp_server" %}

since I want to know how to resolve this conflict because I think it is redundant if the --net is not in dhcp_server docker manifest.

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Feb 7, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@azmy98 azmy98 force-pushed the sonic_docker_image_ctl_update branch from 9b057a1 to 72d83e5 Compare August 12, 2024 09:44
@azmy98
Copy link
Copy Markdown
Author

azmy98 commented Sep 10, 2024

/azpw run Azure.sonic-utilities

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run Azure.sonic-utilities

@azure-pipelines
Copy link
Copy Markdown

No pipelines are associated with this pull request.

@azmy98
Copy link
Copy Markdown
Author

azmy98 commented Sep 10, 2024

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Copy Markdown
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@fastiuk
Copy link
Copy Markdown
Contributor

fastiuk commented Oct 8, 2024

@qiluo-msft , could you please merge it? It was approved, so now ready to be merged

@fastiuk
Copy link
Copy Markdown
Contributor

fastiuk commented Oct 14, 2024

@qiluo-msft , could you please approve this PR? It was approved

@fastiuk
Copy link
Copy Markdown
Contributor

fastiuk commented Oct 21, 2024

@qiluo-msft gentle reminder to merge this PR

1 similar comment
@fastiuk
Copy link
Copy Markdown
Contributor

fastiuk commented Jan 6, 2025

@qiluo-msft gentle reminder to merge this PR

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.

7 participants