Skip to content
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

Implement default cache with a minimal set of package-level functions. #192

Merged
merged 5 commits into from
Mar 1, 2024

Conversation

klihub
Copy link
Contributor

@klihub klihub commented Jan 23, 2024

This patch set

  • reintroduces the implicitly available default cache
  • adds a minimal set of package level functions (Configure(), Refresh(), InjectDevices(), GetErrors()) which use the default cache
  • adds a GetDefaultCache() getter to allow other cache functions to be explicitly called on the default cache
  • updates the registry to use the default cache instead of creating its own instance
  • adds tests for the default cache
  • adds a documentation note about future plans to obsolete and eventually remove the registry abstraction
  • update the cdi test utility to use the default cache

@klihub klihub requested review from elezar and marquiz January 23, 2024 17:21
@klihub klihub force-pushed the devel/reintroduce-default-cache branch 2 times, most recently from 22ada99 to c7d4144 Compare January 24, 2024 12:44
@klihub klihub marked this pull request as ready for review January 24, 2024 12:52
@klihub klihub force-pushed the devel/reintroduce-default-cache branch from c7d4144 to 8b0f137 Compare January 24, 2024 13:42
@klihub
Copy link
Contributor Author

klihub commented Jan 25, 2024

@elezar @marquiz I threw together this PR based on our last community meeting discussion regarding adding top-level package functions (and an implicitly available default cache instance) for the most commonly used functionality. PTAL.

@klihub
Copy link
Contributor Author

klihub commented Jan 31, 2024

@elezar @marquiz @kad ping

@klihub klihub requested a review from kad January 31, 2024 07:00
Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about the default-cache package-level api: should it expose all methods of Cache as package level functions? If not, should it match the current registry API, then?

pkg/cdi/registry.go Show resolved Hide resolved
Copy link
Contributor

@elezar elezar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @klihub.

I have left some comments. I think we can already remove some of the "extra" registry functions from the Registry interface -- or at least mark these as deprecated.

On your testing question: What are we trying to test by also testing the default functions? Should the tests for the default cache be a separate set of test cases anyway?

cmd/cdi/cmd/cdi-api.go Show resolved Hide resolved
pkg/cdi/cache_test.go Outdated Show resolved Hide resolved
pkg/cdi/registry.go Show resolved Hide resolved
pkg/cdi/default-cache.go Outdated Show resolved Hide resolved
@klihub
Copy link
Contributor Author

klihub commented Jan 31, 2024

Question about the default-cache package-level api: should it expose all methods of Cache as package level functions? If not, should it match the current registry API, then?

Based on our last community meeting chat my understanding was that the preference would be to only expose a minimal set of functions at this point, and this set would be limited to cache refreshing, device injection and error introspection/query. And the reasoning was that this would allow us to replace the current usage of the registry in cri-o and containerd with the package-level functions, which would be the next step we'd like to take.

@klihub
Copy link
Contributor Author

klihub commented Feb 1, 2024

Thanks @klihub.

I have left some comments. I think we can already remove some of the "extra" registry functions from the Registry interface -- or at least mark these as deprecated.

On your testing question: What are we trying to test by also testing the default functions?

Yes, that's what I'm also pondering now. If the cache implementation works then as longer as the getter works correctly and the straightforward package-level oneliner functions are not broken, then the default cache should work as well. However, I'm a pessimist-realist so I think we should get these exercised anyway, if for nothing else then for coverage %.

Should the tests for the default cache be a separate set of test cases anyway?

Yes, I think that ideally there should at least be separate top-level test cases (Test$CASE t* testing.T) for the default cache functions. But it should be fine for those to be totally minimalistic wrt. the corresponding functions testing the cache (even as simple as simply checking only for successful injection without verifying the correctness of the result which we already do separately for the Cache tests).

@klihub klihub force-pushed the devel/reintroduce-default-cache branch 2 times, most recently from 383cd76 to 2985980 Compare February 6, 2024 14:48
@klihub klihub marked this pull request as draft February 6, 2024 14:49
@klihub
Copy link
Contributor Author

klihub commented Feb 6, 2024

@elezar @marquiz I pushed an updated version with some of the test-related comments addressed. However, I'd like to add a few more tests for the default cache, so I re-marked this as a draft until I get those done.

@klihub klihub force-pushed the devel/reintroduce-default-cache branch from 2985980 to 370e48b Compare February 20, 2024 12:25
@klihub klihub marked this pull request as ready for review February 20, 2024 12:26
@klihub
Copy link
Contributor Author

klihub commented Feb 20, 2024

Thanks @klihub.
I have left some comments. I think we can already remove some of the "extra" registry functions from the Registry interface -- or at least mark these as deprecated.
On your testing question: What are we trying to test by also testing the default functions?

Yes, that's what I'm also pondering now. If the cache implementation works then as longer as the getter works correctly and the straightforward package-level oneliner functions are not broken, then the default cache should work as well. However, I'm a pessimist-realist so I think we should get these exercised anyway, if for nothing else then for coverage %.

Should the tests for the default cache be a separate set of test cases anyway?

Yes, I think that ideally there should at least be separate top-level test cases (Test$CASE t* testing.T) for the default cache functions. But it should be fine for those to be totally minimalistic wrt. the corresponding functions testing the cache (even as simple as simply checking only for successful injection without verifying the correctness of the result which we already do separately for the Cache tests).

I created dedicated tests for the default cache.

@klihub klihub requested a review from elezar February 20, 2024 13:35
@klihub klihub force-pushed the devel/reintroduce-default-cache branch from 370e48b to 858150c Compare March 1, 2024 13:12
klihub added 5 commits March 1, 2024 15:23
The default cache is always available and implicitly created the
first time it is referenced. It is created with the default cache
options but can then be reconfigured explicitly if necessary.

Aso add package level variants of the most commonly used cache
functions, Refresh(), InjectDevices, and GetErrors(). These all
use the default cache.

Signed-off-by: Krisztian Litkey <[email protected]>
Make the registry use the default cache. Add a documentation note
about future plans to obsolete and eventually remove the registry
interface. Don't mark the registry obsoleted yet.

Signed-off-by: Krisztian Litkey <[email protected]>
Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub force-pushed the devel/reintroduce-default-cache branch from 858150c to 07fed87 Compare March 1, 2024 13:24
@klihub
Copy link
Contributor Author

klihub commented Mar 1, 2024

@elezar I updated the PR according to your suggestion/our earlier discussion.

Another change I made is to remove the returned error from the package-level Configure(), since Cache.Configure() always returns nil since a few PRs ago. We can't do a backward-incompatible signature change to Cache.Configure() now, but I think in v0.7.0 we should do that.

@klihub klihub requested review from elezar and marquiz March 1, 2024 13:28
Copy link
Contributor

@elezar elezar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @klihub. I think this looks good now.

As discussed, let's mark the Registry interfaces and the GetRegistry function as deprecated as a follow-up.

Users should use the top-level functions in most cases, and call Configure() or GetRegistry depending on whether they want to initialize or configure the cache.

@elezar elezar merged commit 2c40f2d into cncf-tags:main Mar 1, 2024
7 checks passed
@klihub klihub deleted the devel/reintroduce-default-cache branch March 1, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants