cdi: add resolved edits API#322
Conversation
|
Thanks, @klihub — your comments made the result API clearer. I’ve updated the resolved device-node and mount outputs to embed the effective CDI types with scope metadata, and added coverage for the FIFO zero-value convention. |
| import cdi "tags.cncf.io/container-device-interface/specs-go" | ||
|
|
||
| // ResolvedEdits contains CDI edits resolved without applying OCI semantics. | ||
| type ResolvedEdits struct { |
There was a problem hiding this comment.
My first thought here is why we need a new type and can't just return the appended ContainerEdits? I'm not saying that distinguishing between supported and unsupported edits is not useful, but I would argue that this is something that the CLIENT must decide and not be hard-coded into the CDI API implementation.
There was a problem hiding this comment.
I think the new version is more what you had in mind?
Signed-off-by: Ehren Bendler <ebendler@nvidia.com>
| func (c *Cache) ResolveEdits(devices ...string) (*ContainerEdits, error) { | ||
| c.Lock() | ||
| defer c.Unlock() | ||
|
|
||
| if _, err := c.refreshIfRequired(false); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| selected, unresolved := c.collectContainerEdits(devices) | ||
| if len(unresolved) > 0 { | ||
| return nil, fmt.Errorf("unresolvable CDI devices %s", strings.Join(unresolved, ", ")) | ||
| } | ||
|
|
||
| resolved := cloneContainerEdits(nil) | ||
| for _, entry := range selected { | ||
| copy := cloneContainerEdits(entry) | ||
| if err := copy.resolveDeviceNodes(); err != nil { | ||
| return nil, err | ||
| } | ||
| resolved.Append(copy) | ||
| } | ||
|
|
||
| return resolved, nil | ||
| } |
There was a problem hiding this comment.
We could decide whether we want to release the lock after we have cloned the collected ContainerEdits.
|
|
||
| // ResolveEdits resolves the given qualified devices to their effective CDI | ||
| // edits without applying OCI conversion or mutating an OCI Spec. | ||
| func (c *Cache) ResolveEdits(devices ...string) (*ContainerEdits, error) { |
There was a problem hiding this comment.
Question: Should this return a cdi.ContainerEdits type instead?
| resolved := cloneContainerEdits(nil) | ||
| for _, entry := range selected { | ||
| copy := cloneContainerEdits(entry) | ||
| if err := copy.resolveDeviceNodes(); err != nil { | ||
| return nil, err | ||
| } | ||
| resolved.Append(copy) | ||
| } |
There was a problem hiding this comment.
Instead of manually implementing clone functions that would need to be updated for every added non-value field, what about constructing a copy by marshalling to JSON and then unmarshalling again?
| selected, unresolved := c.collectContainerEdits(devices) | ||
| if len(unresolved) > 0 { | ||
| return nil, fmt.Errorf("unresolvable CDI devices %s", strings.Join(unresolved, ", ")) | ||
| } | ||
|
|
||
| resolved := cloneContainerEdits(nil) | ||
| for _, entry := range selected { | ||
| copy := cloneContainerEdits(entry) | ||
| if err := copy.resolveDeviceNodes(); err != nil { | ||
| return nil, err | ||
| } | ||
| resolved.Append(copy) | ||
| } |
There was a problem hiding this comment.
What about:
- call
collectContainerEdits - Use the same flow to construct a single
ContainerEditsthat we use inInjectDevices - Marshal the collected edits to JSON.
- Unmarshal the selected edits.
- Fill missing info for included device nodes.
This also seems to indicate that collectContainerEdits should be:
func (c *Cache) collectContainerEdits(devices []string) (*ContainerEdits, error) {
...
}
and include its current content as well as unresolved error handling and looping over the resolved edits.
The idea here is that some CDI consumers may not be operating on an OCI container (think sandbox wrappers), but it would be silly to have them all re-implement the code that maps the specfile to the running host when the reference implementations are already doing all of that work.
See also: cncf-tags/container-device-interface-rs#101