Clean up/update CDI API. #39

klihub · 2021-11-29T16:01:36Z

This is an implementation of the API cleanup/update mentioned in #38 .

elezar

Thanks for the work here! Some initial comments. I will continue tomorrow.

One general comment, it may be useful to remove the "auto" refresh of the cache from this change to simplify the API somewhat. The monitor utility added could still implement this in a loop.

cmd/cdi/LICENSE

go.mod

pkg/cdi/container-edits.go

pkg/cdi/container-edits_test.go

pkg/cdi/container-edits.go

pkg/cdi/registry.go

pkg/cdi/cache.go

klihub · 2021-12-01T00:07:01Z

Thanks for the work here! Some initial comments. I will continue tomorrow.

One general comment, it may be useful to remove the "auto" refresh of the cache from this change to simplify the API somewhat. The monitor utility added could still implement this in a loop.

Yes, I concluded to the same based on our discussion in the meeting so I already did exactly that.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

Implement composing, parsing and validating qualified device names and their vendor, class and device components. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

Implement container edit validation, applying container edits to OCI Specs. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

Implement loading and validation of specs and devices. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

Implement discovery and caching of spec files and devices, refreshing the cache. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

Implement registry, related interfaces and functions. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

elezar

Sorry for the delay in getting to this @klihub. I am currently on PTO.

I think the changes are already a lot cleaner from an API perspective with the utility / extra functionality provided by the CLI. I would also like to get @klueska's input here if that's ok.

elezar · 2021-12-14T13:37:18Z

pkg/cdi/cache.go

+// InjectDevices injects the given qualified devices to an OCI Spec. It
+// returns any unresolvable devices and an error if injection fails for
+// any of the devices.
+func (c *Cache) InjectDevices(ociSpec *oci.Spec, devices ...string) ([]string, error) {


Does it really make sense to have InjectDevices associated with a "cache"? The storage of the specs and applying the required OCI modifications seem like different (orthogonal) concerns.

This is the last open question that I have.

This assumes that the client has instantiated a Cache and is performing all injections for a spec through this. From my perspective it is more intuitive for a Cache to essentially be a structure that allows a user to obtain a CDI spec for a given device ID. This spec object can then be used to apply edits to the OCI spec. What is the motivation for doing the lookup and modification in a single method associated with the Spec store?

From the code below it seems that one concern is that we may be applying the same spec modifications for different devices and that collecting this in a single method allows us to keep track of this (as well as ensure that the cache is locked while making updates). If applying duplicate edits is a concern, then I would also expect some form of deduplication of the input device names.

Well, the whole idea here is that if you are a consumer of CDI specs (a runtime or the orchestration), then you

only need to deal with the cache/registry,

can perform commonly needed tasks (injection) with preferably a single function call, and

without having to understand any intricacies of CDI itself

About deduplication of devices to inject vs. deduplication of global-scope container edits from CDI Specs... The latter is an intricacy of CDI itself, so we need/want to take care of it. That situation arises/occurs naturally whenever two or more devices are injected from a single CDI Spec which defines more than one device and has global-scope edits. AFAICT, this is different from deduplicating the consumer-provided set of devices to inject. While that would definitely be also possible, unless there is a common use case where CDI device duplicates arise/occur naturally in the input without special care being taken by the consumer, I think we should just process the input as is without trying to filter duplicates (and let requests with duplicates eventually fail in the worst case, if that happens to be the runtime's behavior).

pkg/cdi/container-edits.go

pkg/cdi/device.go

klihub · 2021-12-14T14:50:21Z

Sorry for the delay in getting to this @klihub. I am currently on PTO.

No problemo.

I think the changes are already a lot cleaner from an API perspective with the utility / extra functionality provided by the CLI. I would also like to get @klueska's input here if that's ok.

Sure, no problem. His input is welcome.

bart0sh

I think this API is a way better than what we currently have. I'd propose to merge it ASAP and update our pending PRs and podman implementation.

elezar · 2021-12-14T19:56:21Z

I think this API is a way better than what we currently have. I'd propose to merge it ASAP and update our pending PRs and podman implementation.

Yes, I agree that this is definitely a huge improvement over what we have. With this in mind, I'm also ok to merge this as part of the next iteration of the API. Since we're still pre-v1 we don't have as strict guarantees to uphold if we do decide to change things.

elezar

Thanks @klihub. One final question / comment. I don't think it's a blocker but would like to get your motivation regarding the InjectDevices method.

cmd/cdi/cmd/cdi-api.go

elezar · 2022-01-10T13:29:12Z

pkg/cdi/cache.go

+// InjectDevices injects the given qualified devices to an OCI Spec. It
+// returns any unresolvable devices and an error if injection fails for
+// any of the devices.
+func (c *Cache) InjectDevices(ociSpec *oci.Spec, devices ...string) ([]string, error) {


This is the last open question that I have.

This assumes that the client has instantiated a Cache and is performing all injections for a spec through this. From my perspective it is more intuitive for a Cache to essentially be a structure that allows a user to obtain a CDI spec for a given device ID. This spec object can then be used to apply edits to the OCI spec. What is the motivation for doing the lookup and modification in a single method associated with the Spec store?

From the code below it seems that one concern is that we may be applying the same spec modifications for different devices and that collecting this in a single method allows us to keep track of this (as well as ensure that the cache is locked while making updates). If applying duplicate edits is a concern, then I would also expect some form of deduplication of the input device names.

Add a sample 'cdi' binary to exercise the API. It can also serve as a CDI command line debug utility for Spec maintainers. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

Add a package-level documentation/overview. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

elezar

Thanks for this @klihub. This is definitely an improvement over the current API.

I think the questions around where exactly we're implementing the InjectDevices method are not blockers and are due to me not having used the API myself. Thanks for all your clarifying discussions.

klihub mentioned this pull request Nov 29, 2021

Proposal to Clean up/Update the CDI API. #38

Closed

klihub force-pushed the draft/api-update-proposal branch 5 times, most recently from 6ff1e37 to 36a5e9a Compare November 29, 2021 19:25

elezar reviewed Nov 30, 2021

View reviewed changes

klihub force-pushed the draft/api-update-proposal branch 2 times, most recently from f108ad7 to d6df4bd Compare December 1, 2021 00:03

klihub force-pushed the draft/api-update-proposal branch 5 times, most recently from 4454a8e to faf66cf Compare December 2, 2021 23:12

klihub added 4 commits December 9, 2021 18:27

specs-go: export OCI type conversion functions.

da449e3

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

pkg/cdi: implement qualified device names.

57b060e

Implement composing, parsing and validating qualified device names and their vendor, class and device components. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

pkg/cdi: add validation and application of edits.

03e5cc4

Implement container edit validation, applying container edits to OCI Specs. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

pkg/cdi: add spec, device loading and validation.

43fc5a0

Implement loading and validation of specs and devices. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

klihub force-pushed the draft/api-update-proposal branch 5 times, most recently from e149207 to f0957c9 Compare December 10, 2021 13:39

klihub changed the title ~~RFC: Proposal to clean up/update CDI API.~~ Clean up/update CDI API. Dec 13, 2021

klihub added 2 commits December 14, 2021 15:00

pkg/cdi: implement cache of specs and devices.

b730453

Implement discovery and caching of spec files and devices, refreshing the cache. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

pkg/cdi: initial registry implementation.

46744e2

Implement registry, related interfaces and functions. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

klihub force-pushed the draft/api-update-proposal branch from f0957c9 to 4da9206 Compare December 14, 2021 13:02

elezar reviewed Dec 14, 2021

View reviewed changes

bart0sh approved these changes Dec 14, 2021

View reviewed changes

klihub marked this pull request as ready for review December 14, 2021 20:04

elezar reviewed Jan 10, 2022

View reviewed changes

klihub added 5 commits January 11, 2022 10:06

cmd/cdi: add sample binary to exercise the API.

a2f3354

Add a sample 'cdi' binary to exercise the API. It can also serve as a CDI command line debug utility for Spec maintainers. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

go.{mod,sum}: update dependencies.

7713923

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

pkg/cdi: add package level documentation.

14f4076

Add a package-level documentation/overview. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

Makefile: add simple Makefile.

b093549

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

.github: update sanity check workflow.

20f04d7

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>

klihub force-pushed the draft/api-update-proposal branch from 4da9206 to 20f04d7 Compare January 11, 2022 08:07

elezar approved these changes Jan 11, 2022

View reviewed changes

elezar merged commit 46367ec into cncf-tags:master Jan 11, 2022

klihub deleted the draft/api-update-proposal branch January 11, 2022 16:57

elezar mentioned this pull request Jan 12, 2022

GPU Support containers/podman#11088

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up/update CDI API. #39

Clean up/update CDI API. #39

klihub commented Nov 29, 2021

elezar left a comment

klihub commented Dec 1, 2021

elezar left a comment

elezar Dec 14, 2021

elezar Jan 10, 2022

klihub Jan 11, 2022 •

edited

Loading

klihub commented Dec 14, 2021

bart0sh left a comment

elezar commented Dec 14, 2021

elezar left a comment

elezar Jan 10, 2022

elezar left a comment

Clean up/update CDI API. #39

Clean up/update CDI API. #39

Conversation

klihub commented Nov 29, 2021

elezar left a comment

Choose a reason for hiding this comment

klihub commented Dec 1, 2021

elezar left a comment

Choose a reason for hiding this comment

elezar Dec 14, 2021

Choose a reason for hiding this comment

elezar Jan 10, 2022

Choose a reason for hiding this comment

klihub Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

klihub commented Dec 14, 2021

bart0sh left a comment

Choose a reason for hiding this comment

elezar commented Dec 14, 2021

elezar left a comment

Choose a reason for hiding this comment

elezar Jan 10, 2022

Choose a reason for hiding this comment

elezar left a comment

Choose a reason for hiding this comment

klihub Jan 11, 2022 •

edited

Loading