[docker_img_ctl.j2] make tmpfs mounts optional and add ability to run container by image id#6439
Merged
liat-grozovik merged 5 commits intosonic-net:masterfrom Mar 16, 2021
Conversation
Signed-off-by: Stepan Blyschak <[email protected]>
Collaborator
|
what is sae dockers? |
Collaborator
Author
|
@lguohan SAE - SONiC Application Extension |
Collaborator
Author
rajendra-dendukuri
suggested changes
Jan 25, 2021
Contributor
rajendra-dendukuri
left a comment
rajendra-dendukuri
left a comment
There was a problem hiding this comment.
I recommend we find a method do the following:
- All dockers mount /tmp if a certain variable is not defined or not set to a certain value
- Dockers which don't want /tmp to be mounted, will define the variable
- The variable is passed to the j2 environment while rendering the docker_image_ctl.j2
This will allow us to minimize the places where /tmp mount logic needs to be specified.
This reverts commit f908b82.
Signed-off-by: Stepan Blyshchak <[email protected]>
Collaborator
Author
|
retest this please |
provided Signed-off-by: Stepan Blyshchak <[email protected]>
qiluo-msft
reviewed
Jan 29, 2021
| {%- if docker_image_name is defined %} | ||
| {{docker_image_name}}:latest \ | ||
| {%- else %} | ||
| {{docker_image_id}} \ |
Collaborator
There was a problem hiding this comment.
Why add docker_image_id? I don't find any clue in your PR description. #Closed
Collaborator
Author
There was a problem hiding this comment.
I updated title and PR description.
With coming app ext feature docker image might not have a 'latest' tag or it might not have a name at all.
So this template is required to accept more general docker_image_id, but preserving compatibility if docker_image_name was passed.
qiluo-msft
requested changes
Jan 29, 2021
| $REDIS_MNT \ | ||
| -v /usr/share/sonic/device/$PLATFORM:/usr/share/sonic/platform:ro \ | ||
| {%- if sonic_asic_platform != "mellanox" %} | ||
| {%- if mount_default_tmpfs is defined and mount_default_tmpfs == "y" %} |
Collaborator
There was a problem hiding this comment.
This and condition is verbose. Is it possible to simplify? #Closed
Collaborator
Author
There was a problem hiding this comment.
Indeed, please check the new way:
{%- if mount_default_tmpfs|default("n") == "y" %}
stepanblyschak
changed the title
Signed-off-by: Stepan Blyschak <[email protected]>
qiluo-msft
approved these changes
Feb 5, 2021
Collaborator
|
@rajendra-dendukuri comments were handled. Can you please review and approve? |
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Collaborator
|
@rajendra-dendukuri your approval is needed. Please provide your feedback. |
rajendra-dendukuri
approved these changes
Mar 12, 2021
raphaelt-nvidia
pushed a commit
to raphaelt-nvidia/sonic-buildimage
that referenced
this pull request
May 23, 2021
… container by image id (sonic-net#6439) - Why I did it I made the docker_img_ctl.j2 applicable for more dockers (including application extensions dockers) by adding an option not to mount tmpfs on /tmp/ and /var/tmp/. In some applications /tmp/ is a different docker volume which can't be tmpfs. Also, I added and ability to pass REPO[:TAG]|[@digest]/IMAGE_ID instead of just REPO name. - How I did it Modified docker_img_ctl.j2 and docker makefiles. - How to verify it Run it on the switch.
carl-nokia
pushed a commit
to carl-nokia/sonic-buildimage
that referenced
this pull request
Aug 7, 2021
… container by image id (sonic-net#6439) - Why I did it I made the docker_img_ctl.j2 applicable for more dockers (including application extensions dockers) by adding an option not to mount tmpfs on /tmp/ and /var/tmp/. In some applications /tmp/ is a different docker volume which can't be tmpfs. Also, I added and ability to pass REPO[:TAG]|[@digest]/IMAGE_ID instead of just REPO name. - How I did it Modified docker_img_ctl.j2 and docker makefiles. - How to verify it Run it on the switch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Stepan Blyschak [email protected]
- Why I did it
I made the docker_img_ctl.j2 applicable for more dockers (including application extensions dockers) by adding an option not to mount tmpfs on /tmp/ and /var/tmp/. In some applications /tmp/ is a different docker volume which can't be tmpfs.
Also, I added and ability to pass REPO[:TAG]|[@digest]/IMAGE_ID instead of just REPO name.
- How I did it
Modified docker_img_ctl.j2 and docker makefiles.
- How to verify it
Run it on the switch.
- Which release branch to backport (provide reason below if selected)
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)