Proposal to Clean up/Update the CDI API.

[klihub]

The current CDI implementation (API) has a number of issues that should be fixed. Among other things:

• API oddities:
  • (exported) functions with arg types that have no functions to acquire instances of those types (e.g. Spec)
  • implementation vs. original intention
    • injection of other than fully qualified types (sometimes) possible, but should not be
    • normal /dev-entries can be 'virtualized', but should not be
  • irregularities (some functions can survive /dev node node names while others error out on them)
• missing functionality
  • injection (or easy querying of) all devices (matching a vendor/class)
• designed for client-side device injection
  • inefficiencies: no caching, CDI directory, Specs rescanned for every single call
  • no atomicity: same as above, with two function calls you can't be sure if you're still operating on the same data set
  • resiliency: an error (syntactic or semantic likewise) causes every operation to fail

There are ongoing efforts to implement CDI device resolution in the runtimes (containerd pull req., draft-NRI plugin), so it would be nice to address most of these issues.

Here is a proposal for an updated API.
It has an almost full implementation of the proposed API.
This is how the API itself looks like.
This commit message lists the proposed functionality.
The proposal also containes a small cobra-based utility which both demonstrates how to use more than just the most basic trivial functions of the API and also allows one to inspect/play around with CDI Specs.

[klihub]

@bart0sh @elezar @kad Here is a proposal for cleaning up/updating the CDI API and adding most of functionality we discussed to be missing in the last COD-gathering.

[elezar]

@klihub does it make sense to open a PR with these changes for discussion? (I am working through the links in the issue description for the time being).

[klihub]

Sure, I can do that. It's definitely easier to have a discussion that might go into some details that way.

[klihub]

@elezar #39 is the draft PR created from/for discussing this.

[elezar]

Just some notes / thoughts for discussion. I still need to go through the PR in more detail.

From a client (container engine) perspective we need to support the following basic operations for a minimally functional implementation:

1. Instantiate a representation of the state of the CDI specs (I think in Clean up/update CDI API. #39 this is a Registry / Cache)
2. Retrieve the CDI spec for a given device (or devices) from this representation based on a fully-qualified ID
3. Apply the modifications for the returned CDI spec(s) to the OCI spec

There are also "advanced" operations that we may need to support:

1. Refreshing the representation (at worst, this can be done client-side by reinstantiating the representation). This is orthogonal to the functionality in the "minimal" case.
2. Lookup devices based on criteria other than a fully-qualified ID. This could be by host-path for the resolve functionality or by globs, for example
3. Filter the registry content based on some criteria

From a CDI spec maintainer perspective there are a number of other operations that would be useful to perform on the representation of the specs. These may include:

1. List the vendors contained in the representation
2. List the devices contained in the representation

From my initial review, the implementation in #39 adds most (if not all) of this functionality. Would we be opposed to breaking this up into multiple PRs instead that focusses on addressing the different use cases separately?

For example, for the first set of functionality discussed we would have to implement (using interfaces for discussion purposes, these could also be concrete types):

type DeviceID string

func NewRegistry() (Registry, error) {

}

type Registry interface {
    // Get returns the cdi.Specs for the specified CDI device IDs.
   Get(...DeviceID) ([]Spec, error) 
}

type Spec interface {
    // ApplyModifications applies the required modifications to the oci Spec
    ApplyModifications(*oci.Spec) error
}

This can then be extended for the other required functionality. For example for filtering, one could also have this implement the Registry interface, wrapping a particular instance and filtering the results instead of attempting to include this in registry functionality directly.

[klihub]

From my initial review, the implementation in #39 adds most (if not all) of this functionality. Would we be opposed to breaking this up into multiple PRs instead that focusses on addressing the different use cases separately?

I'm not opposed to that per se because I do see how that makes reviewing easier. Why I'd like to avoid it is the total lack of any kind of support for stacked PRs in github (one of the most often requested features for ages).

Would it work for you, if instead we split up the commit history to multiple self-contained commits, which would pretty much directly correspond to the functional split/PR series you suggested. It would help reviewing using the commit-by-commit view in github (which is unfortunately far from being perfect) but avoid having to either serialize filing the PRs or having to rebase all subsequent PRs every single time a previous one is touched ?

[elezar]

Splitting the commit history works too. These don't need to be single commits, but the grouping should be clear.

[klihub]

Splitting the commit history works too. These don't need to be single commits, but the grouping should be clear.

Thanks ! I rework the whole shebang to that effect.

[elezar]

@klihub I have merged #39. I think that that should address this issue. Feel free to close this if you think everything is covered. (It may make sense to create new issues for smaller enhancements as we see them become necessary anyway).

[klihub]

Closing as #39 is now merged.