-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Adjusting docker creation to include ports forwarding and docker network type #16248
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
base: master
Are you sure you want to change the base?
Conversation
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. |
@@ -540,7 +540,9 @@ start() { | |||
# TODO: Mellanox will remove the --tmpfs exception after SDK socket path changed in new SDK version | |||
{%- endif %} | |||
docker create {{docker_image_run_opt}} \ | |||
{%- if '--net' not in docker_image_run_opt %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
@qiluo-msft, there is a conflict also on this PR:
can you please direct me to who wrote this new code:
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. |
|
and docker network type
9b057a1
to
72d83e5
Compare
/azpw run Azure.sonic-utilities |
/AzurePipelines run Azure.sonic-utilities |
No pipelines are associated with this pull request. |
/azpw run Azure.sonic-buildimage |
/AzurePipelines run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
@qiluo-msft , could you please merge it? It was approved, so now ready to be merged |
@qiluo-msft , could you please approve this PR? It was approved |
@qiluo-msft gentle reminder to merge this PR |
1 similar comment
@qiluo-msft gentle reminder to merge this PR |
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