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 #223

Merged
merged 62 commits into from
Jun 5, 2024
Merged

SQLite backed cache #223

merged 62 commits into from
Jun 5, 2024

Conversation

moio
Copy link
Contributor

@moio moio commented May 31, 2024

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

Reviewer notes:

  • A lot of this PR is tests - starting with non-testing files at first should simplify review.

  • the main entry point to Steve, ToServer, has been duplicated with a ToServerAlpha func, and with it all subsequent code, so that the existing cache codebase and the SQLite backed cache codebase can coexist in parallel and be activated by feature flags. Therefore:

    • pkg/server/cli/clicontext.go, pkg/server/server.go, pkg/resources/schema.go, pkg/resources/common/formatter.go contain patched copies of the original methods adapted for the new SQLite-backed cache exposed by lasso. The one with non-trivial changes is server.go's setupAlpha
    • all other changes are self contained in the stores/partitionalpha and stores/proxyalpha directories, both intended to be drop in replacements of the original stores/partition and stores/proxy
    • something like kdiff3 ./pkg/stores/proxy ./pkg/stores/proxyalpha and kdiff3 ./pkg/stores/partition ./pkg/stores/partitionalpha can help making sense of differences
      • eg. pkg/stores/proxyalpha/error_wrapper.go, pkg/stores/proxyalpha/watch_refresh.go and pkg/stores/proxyalpha/unformatter.go are exact copies of the originals (plus convenience constructors). Therefore the only relevant file difference is in proxy_store.go

@moio moio requested a review from a team as a code owner May 31, 2024 14:47
moio and others added 29 commits May 31, 2024 17:44
Resources will be fetched by a local SQLite cache instead of directly
from the Kubernetes API, caching will be delegated to SQLite.

Signed-off-by: Silvio Moioli <[email protected]>
Use informer for projectsornamespaces filter

Prior, a NamespaceCache needed to be passed to filter namespaces
belonging to a project. Now, a sqlcache informer is used to
query which namespaces either share a name with a value in the
projectsornamespaces query parameter or belong to a project that
shares a name with one of the values.
@rmweir
Copy link
Contributor

rmweir commented Jun 4, 2024

@MbolotSuse we spoke about this but so its documented here, I think changing how the code is separated (alpha and non-alpha atm) would be fine. Similarly, I think not using the term alpha is fine. This approach was to prevent regression by not commiting any change tot he active, production code-path. A diff could be viewed by PRing a fork where the alpha packages replace their corresponding non-alpha counterparts. Either way I think anything is fine, however you @moio and @moio choose to handle it.

moio added 3 commits June 4, 2024 11:24
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
moio and others added 9 commits June 4, 2024 13:36
Signed-off-by: Silvio Moioli <[email protected]>
Co-authored-by: Michael Bolot <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
Signed-off-by: Silvio Moioli <[email protected]>
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.

One question about the metadata fields. Still reviewing the rest. And one suggestion that's not very important.

main.go Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
pkg/stores/proxyalpha/proxy_store.go Outdated Show resolved Hide resolved
Signed-off-by: Silvio Moioli <[email protected]>
@moio
Copy link
Contributor Author

moio commented Jun 5, 2024

Alpha is not a descriptive name, we should improve that

All prefixes changed to "sql" as suggested.

There's a lot of duplicated code, we should aim to remove that.

Several of the last commits take care of most duplication, especially in code not expected to change. Feel free to point out other cases going forward.

moio added 2 commits June 5, 2024 15:41
Signed-off-by: Silvio Moioli <[email protected]>
@moio
Copy link
Contributor Author

moio commented Jun 5, 2024

Resolved many points, all of those still unresolved have been moved to the follow-up epic issue. Merging.

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.

4 participants