Add initContainer to download model #613
Merged
Conversation
This file contains 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
lianhao
force-pushed
the
model-downloader
branch
from
51ac04f to
8661d19
Compare
eero-t
suggested changes
Nov 27, 2024
|
Has this been tested with k8s volumes with ReadWriteMany where multiple containers update a shared volume? Or is it expected to work similar to the host path case? @lianhao can you comment this? |
lianhao
force-pushed
the
model-downloader
branch
3 times, most recently
from
092b755 to
639d67c
Compare
eero-t
reviewed
Dec 5, 2024
eero-t
reviewed
Dec 5, 2024
lianhao
force-pushed
the
model-downloader
branch
2 times, most recently
from
0efe89e to
dffc70b
Compare
eero-t
suggested changes
Dec 9, 2024
lianhao
force-pushed
the
model-downloader
branch
2 times, most recently
from
55cfcb6 to
9dcc625
Compare
eero-t
approved these changes
Dec 10, 2024
Comment on lines
+39
to
+51
| # Init container sets the downloaded model dir to be group writable, so that container | ||
| # can keep its lock file there. This relies on both containers using the same group ID. |
There was a problem hiding this comment.
I think (with the last change) best place for these comments would be with the runAsGroup check in the deployment's initContainer securityContext, not any more here in values file.
Add initContainer to download model to avoid the permission conflict issue of the .locks directory in the host modeUseHostPath path, when sharing the host model directory between different services. Also always set securityContext in all cases. Signed-off-by: Lianhao Lu <[email protected]>
lianhao
force-pushed
the
model-downloader
branch
from
9dcc625 to
74f9d98
Compare
|
My K8s volume question went unanswered. I guess, I'll be filing issues if it does not work. My bet is that it won't... |
|
@poussa I remembered @yongfengdu has tested the PVC case, but only on NFS storage class if I remembered correctly. |
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Add initContainer to download model to avoid the permission conflict issue of the .locks directory in the host
modeUseHostPathpath, when sharing the host model directory between different services.Issues
n/a.Type of change
List the type of change like below. Please delete options that are not relevant.
Dependencies
List the newly introduced 3rd party dependency if exists.
Tests
Describe the tests that you ran to verify your changes.