-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal to Clean up/Update the CDI API. #38
Comments
@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). |
Sure, I can do that. It's definitely easier to have a discussion that might go into some details that way. |
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:
There are also "advanced" operations that we may need to support:
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:
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 |
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 ? |
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. |
Closing as #39 is now merged. |
The current CDI implementation (API) has a number of issues that should be fixed. Among other things:
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.
The text was updated successfully, but these errors were encountered: