Add support for CDI device configuration #10081
Conversation
sjug
force-pushed
the
cdi_device_lib
branch
2 times, most recently
from
cfa0a39
to
288197b
Compare
sjug
force-pushed
the
cdi_device_lib
branch
3 times, most recently
from
ea52be6
to
bc07c5b
Compare
|
The CI won't be happy until you add a test. |
|
Needs a rebase. |
openshift-ci
bot
added
the
needs-rebase
|
@TomSweeneyRedHat Ack, saw that thanks. Rebased & test added. |
openshift-ci
bot
removed
the
needs-rebase
sjug
force-pushed
the
cdi_device_lib
branch
3 times, most recently
from
1378767
to
f015d7f
Compare
| for _, device := range s.Devices { | ||
| isCDIDevice, err := cdi.HasDevice(device.Path) | ||
| if err != nil { | ||
| // do nothing |
There was a problem hiding this comment.
should we at least log the error (warning or debug level)?
There was a problem hiding this comment.
This "error" will occur whenever the --device passed is not a CDI device.
Agreed it's worth logging somewhere, added.
| if err != nil { | ||
| // do nothing | ||
| } | ||
| if isCDIDevice { |
There was a problem hiding this comment.
in case of errors, what is the value for isCDIDevice?
Would it be safer to use if err == nil && isCDIDevice?
- Persist CDIDevices in container config - Add e2e test - Log HasDevice error and add additional condition for safety Signed-off-by: Sebastian Jug <seb@stianj.ug>
|
@RenaudWasTaken PTAL |
|
|
||
| for _, device := range s.Devices { | ||
| isCDIDevice, err := cdi.HasDevice(device.Path) | ||
| if err != nil { |
There was a problem hiding this comment.
Should we bubble an error up the stack here?
|
cc @umohnani8 |
|
LGTM |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RenaudWasTaken, sjug, umohnani8 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 |
openshift-ci-robot
added
the
approved
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Thanks to Renaud from Nvidia.