-
Notifications
You must be signed in to change notification settings - Fork 55
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
Conversation
8da2b37
to
a81531c
Compare
There was a problem hiding this 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
Signed-off-by: Silvio Moioli <silvio@moioli.net>
There was a problem hiding this 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.
@moio I had some concern about some of the 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. |
Signed-off-by: Silvio Moioli <silvio@moioli.net>
Signed-off-by: Silvio Moioli <silvio@moioli.net>
To be honest, no, but I see your point.
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>
Tests all pass now, dismissing review. |
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>
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>
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.