ilab wrapper: add support for additional mounts #708
Conversation
|
It looks good. Maybe we want to load defaults from a predetermined file? I'm a fan of |
Your exact intention is ambiguous to me, could you please clarify exactly what you mean, perhaps through an example/user story? |
|
LGTM I like the idea of using a defaults file |
User could create a file /etc/defaults/ilab and ilab wrapper would this way user can set variables for In case we want user to run If I understood @kwozyman correctly |
|
That's correct @javipolo that's what I had in mind |
|
@n1hility PTAL |
|
@pastequo What's the deal with the |
omertuc
force-pushed
the
volume
branch
3 times, most recently
from
d74a099 to
106952b
Compare
omertuc
requested review from
MichaelClifford,
rhatdan,
sallyom,
lmilbaum,
cgwalters and
Gregory-Pereira
as code owners
Yes, due to limitations in the release pipeline we will be duplicating what's inside of that folder, one copy for each hardware vendor (meaning one copy for nvidia, another for intel and another for amd) Only until we implement a better solution, but as of now, we decided we can live with duplicated files |
This is a legacy of Red Hat Linux (before it forked into RHEL/Fedora), when there was a sentiment that Use e.g. |
|
Backing up though, we're only using |
Yes, in a future PR, we will temporarily use sudo just for the sake of using the root-owned image
This is something I'm not sure we've considered, could you give an example of what that could look like, and what would be the benefits?
Exactly, this is something we've already been doing even prior to this PR. This PR is just for enabling extra custom mounts on top of that that the user might need
That's a good idea. Maybe we should just do that instead of:
|
|
so now we gonna have 2 config files? One for ilab wrapper and one for instructlab? So we should version the this one as well, otherwilse how do we upgrade? |
Right thats what we need to do. The approach I was discussing with @rhatdan was setting up a uid map and running on behavlf of the user, something like: This would give us rootless behavior, even though rootful |
|
This does require users to have configured /etc/subuid and /etc/subgid, (useradd does this automatically). I take it fetch_subuid is a bash function to read these files. |
right yes hand wavy pseudo-code that would look do something like we could error if they dont have a subuid entry and print instructions or something like that |
# Background We have an ilab wrapper script that users will use to launch the ilab container. Users may want to mount additional volumes into the container, as they could possibly have e.g. large models stored in some external storage. # Problem Users cannot simply edit the script to add the mounts to the podman command as it is read-only. # Solution Add support for an environment variable that users can set to specify additional mounts to be added to the podman command. This will allow users to specify additional mounts without having to modify the script. # Implementation The script will now check for the `ILAB_ADDITIONAL_MOUNTS` environment variable. If it is set, the script will parse the variable as evaluated bash code to get the mounts. The mounts will then be added to the podman command. Example `ILAB_ADDITIONAL_MOUNTS` usage: ```bash ILAB_ADDITIONAL_MOUNTS="/host/path:/container/path /host/path2:/container/path2"` ``` If your path contains spaces, you can use quotes: ```bash ILAB_ADDITIONAL_MOUNTS="/host/path:/container/path '/host/path with spaces':/container/path" ``` The latter works because the script uses `eval` to parse the mounts. Signed-off-by: Omer Tuchfeld <[email protected]>
|
Force pushed to remove /etc/default. A dedicated config file should be discussed separately from this PR |
|
Agree with @omertuc. Let's focus on allowing additional mounts in this PR. |
|
LGTM |
Solves RHELAI-745
Background
We have an ilab wrapper script that users will use to launch the ilab container.
Users may want to mount additional volumes into the container, as they could possibly have e.g. large models stored in some external storage.
Problem
Users cannot simply edit the script to add the mounts to the podman command as it is read-only.
Solution
Add support for an environment variable that users can set to specify additional mounts to be added to the podman command. This will allow users to specify additional mounts without having to modify the script.
Implementation
The script will now check for the
ILAB_ADDITIONAL_MOUNTSenvironment variable. If it is set, the script will parse the variable as evaluated bash code to get the mounts. The mounts will then be added to the podman command.Example
ILAB_ADDITIONAL_MOUNTSusage:If your path contains spaces, you can use quotes:
ILAB_ADDITIONAL_MOUNTS="/host/path:/container/path '/host/path with spaces':/container/path"The latter works because the script uses
evalto parse the mounts.