feat: add cdi path to engine config #1834
Conversation
|
If you add a new field you need to document it in containers.conf file as well as containers.conf.5.md man page. Is this for CRI-O or actually necessary for Podman? If this is just CRI-O then should it be in cri-o.conf file? |
pkg/config/config.go
Outdated
| // Location of CDI configuration files. These define mounts devices and | ||
| // other configs according to the CDI spec. In particular this is used | ||
| // for GPU passthrough. | ||
| CdiSpecDir attributedstring.Slice `toml:"cdi_spec_dir,omitempty"` |
There was a problem hiding this comment.
FWIW we have this option already in CRI-O: https://github.com/cri-o/cri-o/blob/2a20780b7b91fecb27c2663558bb06cc7f817d14/internal/criocli/criocli.go#L963-L968
There was a problem hiding this comment.
@saschagrunert are you saying this can be accomplished with CRI-O config? Or are you saying this should follow the same pattern fo CRIO? Or are you saying this is a useful thing to have?
There was a problem hiding this comment.
should probably rename to cdi-spec-dirs to match
|
My brain read CNI not CDI, sorry about my confusion. Man page and containers.conf changes required though. Add a test also. |
|
@rhatdan first PR.... where are the man page and containers.conf located? |
micahcc
force-pushed
the
mcc/add_cdi_arg
branch
from
b9000ad
to
69bec24
Compare
|
@rhatdan can you take another look? |
|
Please squash your commits and make sure they have proper signature |
|
@vrothberg updated |
pkg/config/config_local_test.go
Outdated
| // Given | ||
| config1, err := New(nil) |
There was a problem hiding this comment.
That looks like a formatting issue. Same for the comments below.
micahcc
force-pushed
the
mcc/add_cdi_arg
branch
2 times, most recently
from
e43da3b
to
1b58bc1
Compare
|
How do I get the LGTM label? |
|
@rhatdan how do I merge this? |
|
@saschagrunert @vrothberg PTAL Needs a second LGTM. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: micahcc, rhatdan, saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@cevich Any idea why this has not merged? |
|
Ahh, yes, I see the problem. This PR is using really old code. Rebase it on main, then CI and the bot will work as expected. Ref. Fix: #1906 |
micahcc
force-pushed
the
mcc/add_cdi_arg
branch
from
1b58bc1
to
7061396
Compare
micahcc
force-pushed
the
mcc/add_cdi_arg
branch
from
7061396
to
bd96e0a
Compare
Adds config to pass CDI spec directory, so that it can be overridden. The enables rootless containers since otherwise users have to write to one of the shared, usually only root writeable paths at in /etc or /var. Signed-off-by: Micah Chambers <[email protected]> Signed-off-by: Micah Chambers (minerva) <[email protected]>
micahcc
force-pushed
the
mcc/add_cdi_arg
branch
from
bd96e0a
to
8019222
Compare
|
@cevich ok, rebased, force pushed with sign 🤞 |
|
I see the fix has now been incorporated. @saschagrunert @rhatdan PTAL again, the bot should behave now. |
|
/lgtm |
In response to:
containers/podman#18292
Adds config to pass CDI spec directory, so that it can be overridden. The enables rootless containers since otherwise users have to write to one of the shared, usually only root writeable paths at in /etc or /var.
Named to match cri-o's name