-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add run and serve arguments for --device and --privileged #809
Conversation
Signed-off-by: Charro Gruver <[email protected]>
Reviewer's Guide by SourceryThis pull request introduces the Sequence diagram for container setup with device argumentsequenceDiagram
participant CLI as Command Line Interface
participant Model as Model
participant ContainerManager as Container Manager
CLI->>Model: run(args)
Model->>Model: setup_container(args)
alt args.device is present
loop for each device in args.device
Model->>ContainerManager: Add --device device_arg to container arguments
end
else args.device is not present
alt System is MacOS or /dev/dri exists
Model->>ContainerManager: Add --device /dev/dri to container arguments
end
end
Model->>ContainerManager: Create and start container
Updated class diagram for CLI argumentsclassDiagram
class ArgumentParser {
+add_argument(name: str, dest: str, action: str, type: str, help: str)
}
class _run {
+--device: str[]
+--privileged: bool
}
ArgumentParser -- _run : Adds arguments to
note for _run "Added --device and --privileged arguments"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @cgruver - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding the
--privileged
argument to theserve
subcommand as well. - It might be helpful to include a brief example of how to use the
--device
argument in the help text.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 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.
@ericcurtin Here's my attempt to complete #572 and add @rhatdan I satisfied your ask to make the arguments part of the subcommands I will add docs for these if we like this approach. |
Looks like I broke the tests. Need to update the appropriate |
Signed-off-by: Charro Gruver <[email protected]>
docs/ramalama-run.1.md
Outdated
@@ -43,6 +46,9 @@ name of the container to run the Model in | |||
#### **--network**=*none* | |||
set the network mode for the container | |||
|
|||
#### **--privileged** | |||
give extended privileges to container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, RamaLama containers are unprivileged (=false) and cannot, for
example, modify parts of the operating system. This is because by de‐
fault a container is only allowed limited access to devices. A "privi‐
leged" container is given the same access to devices as the user launch‐
ing the container, with the exception of virtual consoles (/dev/tty\d+)
when running in systemd mode (--systemd=always).
A privileged container turns off the security features that isolate the
container from the host. Dropped Capabilities, limited devices, read-
only mount points, Apparmor/SELinux separation, and Seccomp filters are
all disabled. Due to the disabled security features, the privileged
field should almost never be set as containers can easily break out of
confinement.
Containers running in a user namespace (e.g., rootless containers) can‐
not have more privileges than the user that launched them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this, from Podman man pages.
docs/ramalama-serve.1.md
Outdated
@@ -46,6 +46,9 @@ The default is TRUE. The --nocontainer option forces this option to False. | |||
|
|||
Use the `ramalama stop` command to stop the container running the served ramalama Model. | |||
|
|||
#### **--device** | |||
declare host device to leak into the container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add something like.
Add a host device to the container. Optional permissions parameter can
be used to specify device permissions by combining r for read, w for
write, and m for mknod(2).
Example: --device=/dev/sdc:/dev/xvdc:rwm.
The device specifiaction is passed directly to the underlying container engine. See documentation of the supported container engine for more information.
docs/ramalama-serve.1.md
Outdated
@@ -70,6 +73,9 @@ set the network mode for the container | |||
#### **--port**, **-p** | |||
port for AI Model server to listen on | |||
|
|||
#### **--privileged** | |||
give extended privileges to container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
ramalama/model.py
Outdated
if device_override != 1: | ||
if args.device: | ||
for device_arg in args.device: | ||
print(device_arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like debug line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops... yes it is... :-)
ramalama/cli.py
Outdated
@@ -233,6 +233,12 @@ def configure_arguments(parser): | |||
help="store AI Models in the specified directory", | |||
) | |||
parser.add_argument("-v", "--version", dest="version", action="store_true", help="show RamaLama version") | |||
# parser.add_argument("--device", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either turn this code on or delete it.
Signed-off-by: Charro Gruver <[email protected]>
LGTM |
Removed
RAMALAMA_DEVICE
for --device override in favor of a--device
argument.Added
--privileged
argument to satisfy the desired outcome of #572Added the ability to pass multiple instances of
--device /dev/some/device
to allow users to explicitly define which devices to leak into the container.--device
and--privileged
are arguments oframalama run|serve
Summary by Sourcery
Add the
--device
and--privileged
options to the run and serve commands. Allow multiple devices to be passed to the--device
argument. Replace theRAMALAMA_DEVICE
environment variable with the new--device
argument.New Features:
--device
and--privileged
arguments to theramalama run
andramalama serve
commands. The--device
argument allows for passing multiple device paths to expose to the container. The--privileged
argument grants extended privileges to the container.RAMALAMA_DEVICE
environment variable with the--device
argument for specifying devices to make available within the container during model serving or running. Multiple--device
arguments can be provided to specify multiple devices