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

SQLite backed cache #65

Merged
merged 41 commits into from
Jun 3, 2024
Merged

SQLite backed cache #65

merged 41 commits into from
Jun 3, 2024

Conversation

rmweir
Copy link
Contributor

@rmweir rmweir commented Apr 17, 2024

Issue:
rancher/rancher#45635
Refers to the Informer-based, SQLite-backed Steve caching ("Vai") RFC

Reviewing:
I recommend starting with the pkg/cache/sql/README.md. If lasso/controller understanding is limited then read the main README.md first. Once you understand of what informer/indexer/indexer's store generally are and how they work, then move on to the those components of the on-disk cache. A lot of this PR is tests- starting with non-testing files at first should simplify review.

Testing:
In addition to passing the unit tests provided in this PR, a version of rancher using this PR has been tested with the integration tests located here.

@rmweir rmweir requested a review from a team as a code owner April 17, 2024 22:23
@rmweir rmweir force-pushed the vai branch 2 times, most recently from 8da2b37 to a81531c Compare April 18, 2024 00:28
@rmweir rmweir requested a review from ericpromislow May 17, 2024 02:38
Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

Only remaining issue is that go test ./... fails with type-related errors in function calls

@moio moio changed the title On-disk SQL cache informer SQLite backed cache May 31, 2024

Verified

This commit was signed with the committer’s verified signature.
zkochan Zoltan Kochan
Signed-off-by: Silvio Moioli <silvio@moioli.net>
Copy link
Contributor

@tomleb tomleb left a comment

Choose a reason for hiding this comment

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

Had some pending comments I never sent. Nit, some clarifying questions, with the assumption that we want to merge and do some more work after in rancher/rancher#45635.

I'd have like to look at the partition bit but Michael already did a good job reviewing there so I'll look at it post-merge.

@tomleb
Copy link
Contributor

tomleb commented May 31, 2024

@moio I had some concern about some of the cache.Indexer / cache.Store methods panicking (ListIndexFuncValues, List and ListKeys). I understand that those interface don't allow us to return an error, which is a bummer, but I'm wondering if panicking is the best we can do here given that this is a library.

Have we thought about other ways of handling these failing cases (eg: returning an empty list)? Is this something we could perhaps revisit in rancher/rancher#45635? Panicking seems pretty brutal for something that could have been SQLite being busy, as an example.

moio and others added 5 commits May 31, 2024 17:49

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: Silvio Moioli <silvio@moioli.net>
Signed-off-by: Silvio Moioli <silvio@moioli.net>
@moio
Copy link
Contributor

moio commented Jun 3, 2024

@moio I had some concern about some of the cache.Indexer / cache.Store methods panicking (ListIndexFuncValues, List and ListKeys). I understand that those interface don't allow us to return an error, which is a bummer, but I'm wondering if panicking is the best we can do here given that this is a library.

Have we thought about other ways of handling these failing cases (eg: returning an empty list)?

To be honest, no, but I see your point.

Is this something we could perhaps revisit in rancher/rancher#45635? Panicking seems pretty brutal for something that could have been SQLite being busy, as an example.

SQLite being busy is addressed by retrying code already, and the number of retries should be so high for that to signal a real bug rather than a temporary condition (or at least, that was the design intent).

But I agree there might be other causes and panic might be inappropriate. I added a point to the epic issue and welcome discussion on alternatives.

-> epic

Signed-off-by: Silvio Moioli <silvio@moioli.net>
@moio
Copy link
Contributor

moio commented Jun 3, 2024

Only remaining issue is that go test ./... fails with type-related errors in function calls

Tests all pass now, dismissing review.

@moio moio dismissed ericpromislow’s stale review June 3, 2024 07:50

tests pass now

@moio moio merged commit 701e919 into rancher:master Jun 3, 2024
1 check passed
moio added a commit to rancher/steve that referenced this pull request Jun 5, 2024
This uses SQLite-backed informers provided by Lasso with rancher/lasso#65 to implement Steve API (/v1/) functionality.

This new functionality is available behind a feature flag to be specified at Steve startup

See https://confluence.suse.com/pages/viewpage.action?pageId=1359086083 

Co-authored-by: Ricardo Weir <ricardo.weir@suse.com>
Co-authored-by: Michael Bolot <michael.bolot@suse.com>
Co-authored-by: Silvio Moioli <silvio@moioli.net>
Signed-off-by: Silvio Moioli <silvio@moioli.net>
krunalhinguu pushed a commit to krunalhinguu/lasso that referenced this pull request Sep 18, 2024
This adds an implementation of a SQLite-backed informer with the ability of querying cached objects flexibly to support Steve's sorting/filtering/pagination needs. The informer also optionally encrypts objects on their way to the disk storage.

See https://confluence.suse.com/pages/viewpage.action?pageId=1359086083 

Co-authored-by: Ricardo Weir <ricardo.weir@suse.com>
Co-authored-by: Silvio Moioli <silvio@moioli.net>
Signed-off-by: Silvio Moioli <silvio@moioli.net>
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.

None yet

7 participants