Repository navigation
Conversation
aad4024 to
c9e99f5
Compare
| for _, deviceID := range utils.DefaultVFDeviceIDs { | ||
| grepArgs = append(grepArgs, "-e "+deviceID) | ||
| for _, deviceID := range getVFIODeviceIDs(devConfig) { | ||
| grepArgs = append(grepArgs, "-e "+quoteShellArg(deviceID)) |
There was a problem hiding this comment.
[P2] Use consistent PCI-ID matching in detection and binding
A valid custom ID such as deviceIDs: ["74B5"] or ["0x74b5"] now leaves the worker stuck in its init container. The binding command’s lspci -d 1002:<ID> accepts both forms, but lspci -nn prints [1002:74b5], so the new case-sensitive grep never matches. Previously the default detector matched 74b5, allowing these configurations to proceed. Please use the same PCI-ID selection semantics for detection and binding, for example by checking for nonempty lspci -d output, and cover uppercase and prefixed IDs in regression tests.
There was a problem hiding this comment.
🟡 Changes recommended
Unquoted PCI ID arguments allow configured text to alter host-device selection, while case-sensitive detection fails for uppercase IDs and prevents the binding container from starting.
2 open issues
What changed in this PR
This PR tightens PF/VF passthrough configuration validation and makes worker commands use configured VFIO device IDs consistently. GIM artifact resolution and the separate unbind shell-test fix remain out of scope.
Changes:
- Require a nonempty node selector for enabled PF/VF passthrough, treating an omitted enable flag as enabled.
- Centralize VFIO ID selection and shell-quote IDs in generated binding and detection commands.
- Add tests for selector validation, custom-ID propagation, and shell metacharacter quoting.
| File | Description |
|---|---|
internal/validator/specValidators.go |
Require selectors for enabled passthrough configurations. |
internal/validator/specValidators_test.go |
Cover selector requirements and unchanged container-driver validation. |
internal/controllers/workermgr/workermgr.go |
Share VFIO ID selection and quote IDs in generated commands. |
internal/controllers/workermgr/workermgr_test.go |
Check custom-ID quoting in binding and VF detection commands. |
Review effort: Balanced
| } | ||
| quotedDeviceIDs := make([]string, 0, len(deviceIDs)) | ||
| for _, deviceID := range deviceIDs { | ||
| quotedDeviceIDs = append(quotedDeviceIDs, quoteShellArg(deviceID)) |
| for _, deviceID := range utils.DefaultVFDeviceIDs { | ||
| grepArgs = append(grepArgs, "-e "+deviceID) | ||
| for _, deviceID := range getVFIODeviceIDs(devConfig) { | ||
| grepArgs = append(grepArgs, "-e "+quoteShellArg(deviceID)) |
Problem
The operator supports PF/VF passthrough through a node-scoped DeviceConfig and VFIO device IDs. Two generic correctness and safety gaps make those configurations unsafe or incomplete:
spec.driver.vfioConfig.deviceIDs, the bind/unbind command uses those IDs but the VF-detection init container still scans only the built-in defaults. A custom VF configuration can therefore wait indefinitely for a device that the detection command does not look for.What this changes
spec.selectorfor enabledvf-passthroughandpf-passthroughconfigurations. The validation treats an omitteddriver.enableas enabled, matching the CRD default.Compatibility
This PR is independent of GIM artifact resolution. It applies only to PF/VF passthrough configurations; container-driver behavior is unchanged. The AMD MI3xx default VF IDs and GIM artifact mapping are kept in PR #674. The unrelated
-efix remains in PR #673.Testing
GOTOOLCHAIN=auto go test ./api/... ./cmd/... ./internal/... ./tools/build/copyrightGOTOOLCHAIN=auto go vet ./api/... ./cmd/... ./internal/... ./tools/build/copyrightgit diff --checkThe tests verify that configured VFIO IDs appear safely in both the binding command and detection command, and that enabled PF/VF passthrough rejects an empty selector.
Out of scope