ilab wrapper: add support for additional mounts

[omertuc]

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_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:

ILAB_ADDITIONAL_MOUNTS="/host/path:/container/path /host/path2:/container/path2"`

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 eval to parse the mounts.

[kwozyman]

It looks good. Maybe we want to load defaults from a predetermined file? I'm a fan of /etc/default files (for example, etc/default/ilab)

[omertuc]

It looks good. Maybe we want to load defaults from a predetermined file? I'm a fan of /etc/default files (for example, etc/default/ilab)

Your exact intention is ambiguous to me, could you please clarify exactly what you mean, perhaps through an example/user story?

[javipolo]

LGTM

I like the idea of using a defaults file

[javipolo]

It looks good. Maybe we want to load defaults from a predetermined file? I'm a fan of /etc/default files (for example, etc/default/ilab)

Your exact intention is ambiguous to me, could you please clarify exactly what you mean, perhaps through an example/user story?

User could create a file /etc/defaults/ilab

and ilab wrapper would source /etc/defaults/ilab if it exists

this way user can set variables for ilab wrapper in a very simple way

In case we want user to run ilab as a user, we could also source $HOME/.config/ilab if it exists

If I understood @kwozyman correctly

[kwozyman]

That's correct @javipolo that's what I had in mind

[rhatdan]

@n1hility PTAL

[omertuc]

@pastequo What's the deal with the duplicated folder? Should edits to the wrapper be made in both locations?

[javipolo]

@pastequo What's the deal with the duplicated folder? Should edits to the wrapper be made in both locations?

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

[cgwalters]

/etc/defaults/

This is a legacy of Red Hat Linux (before it forked into RHEL/Fedora), when there was a sentiment that /etc was a mess and we'd create a new, cleaner thing. That basically didn't happen in any broad sense. Just for fun, debian has /etc/default (without the s) that's only got a single legacy thing there too in the default container.

Use e.g. /etc/ilab or so instead, no need to use the legacy "we put an etc in your etc".

[cgwalters]

Backing up though, we're only using sudo temporarily as a way to use the root-owned image, right? But certainly we could still set up that container so that it continues to run as the invoking UID by default, and mount /home to the user's home. That would make it clearer to also support per-user config in e.g. ~/.config/ilab.conf or whatever.

[omertuc]

Backing up though, we're only using sudo temporarily as a way to use the root-owned image, right?

Yes, in a future PR, we will temporarily use sudo just for the sake of using the root-owned image

But certainly we could still set up that container so that it continues to run as the invoking UID by default,

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?

and mount /home to the user's home

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 would make it clearer to also support per-user config in e.g. ~/.config/ilab.conf or whatever.

That's a good idea. Maybe we should just do that instead of:

Use e.g. /etc/ilab or so instead, no need to use the legacy "we put an etc in your etc".

[romfreiman]

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?

[n1hility]

Backing up though, we're only using sudo temporarily as a way to use the root-owned image, right? But certainly we could still set up that container so that it continues to run as the invoking UID by default, and mount /home to the user's home. That would make it clearer to also support per-user config in e.g. ~/.config/ilab.conf or whatever.

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:

---uidmap 0:$UID --uidmap 1:$(fetch_subuid):65534 --v $HOME:/root

This would give us rootless behavior, even though rootful

[rhatdan]

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.

[n1hility]

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

$(awk -F ':' -v user="$(id -un)" '$1 == user {print $2}' /etc/subuid)

we could error if they dont have a subuid entry and print instructions or something like that

[omertuc]

Force pushed to remove /etc/default. A dedicated config file should be discussed separately from this PR

[fabiendupont]

Agree with @omertuc. Let's focus on allowing additional mounts in this PR.

[rhatdan]

LGTM