Add logic to build intel-gpu image to build_llama_and_whisper.sh #724
Conversation
Signed-off-by: Charro Gruver <cgruver@redhat.com>
cgruver
requested review from
rhatdan,
ericcurtin,
bmahabirbu,
maxamillion,
dougsland,
swarajpande5,
jhjaggars,
slp and
engelmi
as code owners
Reviewer's Guide by SourceryThis pull request modifies the intel-gpu container image to use the common build script and adds an entrypoint to set the environment. Sequence diagram for intel-gpu container startup processsequenceDiagram
participant Container as Container Start
participant Entrypoint as Entrypoint Script
participant System as System Config
Container->>Entrypoint: Start container
Entrypoint->>System: Check/Create HOME directory
Entrypoint->>System: Configure user in /etc/passwd
Entrypoint->>System: Add user to render/video groups
Entrypoint->>System: Configure shell (.bashrc/.zshrc)
Entrypoint->>System: Source Intel OneAPI environment
Entrypoint->>Container: Execute CMD
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cgruver - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| sed "s|${render_group}|${render_group_new}|g" /etc/group > /tmp/group | ||
| cat /tmp/group > /etc/group |
There was a problem hiding this comment.
🚨 issue (security): The group file modifications could race when multiple containers start simultaneously
Consider using a more atomic approach to modify the group file. Also, the USER_NAME variable should be properly escaped to prevent potential injection issues.
There was a problem hiding this comment.
Yeah:
mv /tmp/group /etc/group
would be more atomic, same on line 28.
I don't see the race condition myself, but mv is a nicer option, if it works, more atomic to replace the inode.
There was a problem hiding this comment.
You're right that mv /tmp/group /etc/group is more atomic and generally a better approach for replacing the file. However, the race condition I'm referring to isn't about replacing the file itself, but about concurrent modifications to the file. If multiple containers based on this image start simultaneously, they could all be reading /etc/group, modifying it in memory, and then writing their modified version back. This could lead to some updates being lost.
For example:
- Container 1 reads
/etc/group. - Container 2 reads
/etc/group(same content as Container 1). - Container 1 adds
llama-userto therendergroup and writes the updated/etc/group. - Container 2 adds
llama-userto thevideogroup and writes the updated/etc/group, overwriting the changes made by Container 1 regarding therendergroup.
Using mv doesn't solve this. A more robust solution would be to use a tool designed for concurrent file modifications, like usermod and groupmod. These tools handle locking and ensure that changes aren't overwritten. They also address the escaping concern I mentioned.
Could you try using usermod -a -G render ${USER_NAME} and usermod -a -G video ${USER_NAME} instead of directly manipulating /etc/group? This would be safer and more idiomatic. Also, ensure that USER_NAME is properly defined and sanitized before being used in these commands.
There was a problem hiding this comment.
Wouldn't each container have it's own filesystem anyway? Hence no race condition
There was a problem hiding this comment.
You're right that each container has its own filesystem, after it starts. However, the /etc/group file is typically part of the container image. So, when multiple containers are launched from the same image, they initially share the same /etc/group file. The modifications made by the entrypoint script happen after the container starts but before any user-specified commands run.
So, if multiple containers based on this image start simultaneously, they will all run this entrypoint script concurrently, and they will all be modifying their initially-shared copy of /etc/group. This is where the race condition occurs. Even though they each have their own filesystem later, the changes made during startup can be overwritten by other containers starting at the same time.
Using tools like usermod and groupmod is crucial because they handle file locking and ensure that concurrent modifications to /etc/group are applied correctly, avoiding the race condition even when multiple containers start simultaneously from the same image. They also handle escaping and other potential security issues related to directly manipulating /etc/group.
There was a problem hiding this comment.
I tried mv initially but it requires rw access to the underlying folder, where cat > just requires rw on the file.
|
The CI is broken outside of this PR. |
Ah, I was digging through logs trying to figure out what I had broken... Thanks! |
Modify the Containerfile for intel-gpu to use the common build script.
Add entrypoint to intel-gpu image to set environment.