Skip to content
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

[WIP] feat: add cdi-spec-dir option to top level options #21448

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

micahcc
Copy link

@micahcc micahcc commented Jan 31, 2024

In response to #18292.

One of the primary benefits of podman is that its possible run without altering the host system. By allowing cdi directories to be passed via command line it maintains this feature.

I'm particularly interested in running podman in a nix-shell with nvidia GPU passthough. Podman and all its dependencies (including nvidia-container-toolkit) come from nix and the host system again doesn't need altering. The one problem with that right now is that I have to write /etc/cdi/nvidia.yaml to be able to use the GPU. Note that the old nvidia-runtime-hook does not work for my use case (which involves vulkan passthrough).

With this change it will be possible to generate a user nvidia.yaml and point podman at that user-specific mount instructions there.

Does this PR introduce a user-facing change?

- Add cdi-spec-dir option to insert additional CDI Spec search paths into the CDI loader.

Copy link
Contributor

openshift-ci bot commented Jan 31, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: micahcc
Once this PR has been reviewed and has the lgtm label, please assign flouthoc for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@micahcc
Copy link
Author

micahcc commented Jan 31, 2024

Needs: containers/common#1834

@rhatdan rhatdan changed the title feat: add cdi-spec-dir option to top level options [WIP] feat: add cdi-spec-dir option to top level options Jan 31, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2024
Copy link

github-actions bot commented Mar 2, 2024

A friendly reminder that this PR had no activity for 30 days.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2024
Micah Chambers (eos) added 2 commits April 30, 2024 20:49
Signed-off-by: Micah Chambers (eos) <mchambers@anduril.com>
Signed-off-by: Micah Chambers (eos) <mchambers@anduril.com>
@micahcc micahcc force-pushed the mcc/add_cdi_arg branch from 1a05985 to 55b61ab Compare May 1, 2024 03:51
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2024
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@abergeron
Copy link

Is there any intention of merging this PR?

I did try to use the cdi_spec_dirs option for a project only to spend days wondering what I did wrong as it didn't work and eventually ending up here to realize that it doesn't work because it was never hooked up in the code to do something.

As best as I understand, this PR is what would make it do that something.

@isbecker
Copy link

@micahcc are you able to move this forward now?
The required containers/common#1834 is merged upstream and even #25171 was merged for CDI support with podman-compose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note stale-pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants