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

Support enabling SQLite extensions #30

Open
otoolep opened this issue Aug 15, 2024 · 20 comments
Open

Support enabling SQLite extensions #30

otoolep opened this issue Aug 15, 2024 · 20 comments
Assignees

Comments

@otoolep
Copy link
Member

otoolep commented Aug 15, 2024

The latest Docker images support enabling SQLite extensions which are part of the Docker image. Perhaps the Helm charts should support this?

See https://rqlite.io/docs/guides/extensions/#docker for more details.

@jtackaberry jtackaberry self-assigned this Aug 16, 2024
@jtackaberry
Copy link
Collaborator

jtackaberry commented Aug 16, 2024

@otoolep It's already possible to pass extra environment variables to rqlite with the chart using extraEnv. For example by adding to the YAML values:

extraEnv:
  - name: SQLITE_EXTENSIONS
    value: sqlean icu

And it's also easy to pass extra arguments to rqlite:

extraArgs:
  - -extensions-path=/opt/rqlite/extensions

Do you think the chart should provide a specific abstraction for extensions, and just prepare SQLITE_EXTENSIONS in behind the scenes? Something like this?

extensions:
  - sqlean
  - icu

@otoolep
Copy link
Member Author

otoolep commented Aug 17, 2024

TL;DR: I like your suggestion, but it's important to call out that it would only apply to extensions that are built-in to the Docker image.

So this is my sense of what we should do:

  • The extensions functionality is useful (and significant?) enough to warrant its own section in the Helm files, as you suggest via extensions. It really does introduce a bunch of new useful functionality, and I hope to bake more extensions into the Docker image as time goes by.
  • The way docker-entrypoint is structured -- at least right now -- is that by setting the enviroment variable it overrides any explicit setting of -extensions-path. So, in that sense, this environment variable says "load these preloaded extensions in the Docker image, but no others". This is probably important to call out. So your suggestion for a dedicated extensions section is what I had in mind, but with the caveat it's only for those extensions baked into the image.
  • If someone wanted to load their own special extension I expect they would mount a file for the Docker container to access, and set -extension-path explicitly, and not touch the extensions section of the Helm chats,. But that's an advanced use case.

@otoolep
Copy link
Member Author

otoolep commented Aug 17, 2024

I have this sense that these extensions are very useful, and will become more useful over time. So I think signal-boosting their existence via a dedicated extensions section in the Helm charts (even if that section is basically syntatic sugar) helps our users.

@jtackaberry
Copy link
Collaborator

The extensions functionality is useful (and significant?) enough to warrant its own section in the Helm files, as you suggest via extensions. It really does introduce a bunch of new useful functionality, and I hope to bake more extensions into the Docker image as time goes by.

Might extensions take custom configuration at some point? Can you anticipate future needs for extensions that could influence how we structure the chart values? That is, I'm wondering if extensions should be just a straight string list as in my first reply, or a map allows for more flexibility to address future use cases, such as:

extensions:
  enabled:
    - sqlean
    - icu
  config:
    sqlean:
      whoknows: 42

Or perhaps:

extensions:
  sqlean:
    enabled: true
    config:
      whoknows:42
  icu:
    enabled: true

If you have any plans for future enhancements that might influence this, we might avoid a future breaking change to the chart (or janky/unintuitive configuration).

Possibly there's no need for extension-specific configuration, so this is over-complicated.

This is probably important to call out. So your suggestion for a dedicated extensions section is what I had in mind, but with the caveat it's only for those extensions baked into the image.

Is there a way to enable both built-in (baked into the official image) extensions and custom extensions? It actually sounds like that's not possible, given what you said. Perhaps if -extensions-path accepted a colon delimited list of paths, then one could point rqlite to both its built-in extensions as well as any user-custom extensions?

I see in the image each extension is a separate directory under /opt/extensions. The entrypoint script used by the chart could discover directories under /opt/extensions, and assemble an appropriate colon-delimited path string based on the enabled extensions in the extensions chart value. Plus any user-defined custom paths that contain user's custom extensions.

Suppose the chart values has:

extensions:
  enabled:
    - sqlite-vec
    - icu
    - myextension
  customPaths:
    - /lib/sqlite/extensions

The chart's custom entrypoint would then figure out that it needs to pass -extensions-path=/lib/sqlite/extensions:/opt/extensions/sqlite-vec:/opt/etensions/icu. Of course assuming rqlite did eventually support multiple paths using this colon-delimited approach.

extensions.customPaths is perhaps a little unnecessary, because ...

If someone wanted to load their own special extension I expect they would mount a file for the Docker container to access, and set -extension-path explicitly, and not touch the extensions section of the Helm chats,. But that's an advanced use case.

... without an internet-facing extensions registry (like we discussed over email a la Grafana's plugins) from which extensions could be fetched at startup, the way I would handle this is just to build my own custom rqlite image, doing a multistage build to first compile the extensions I want, and then another stage that bases off of docker.io/rqlite/rqlite and just copies my custom extensions to /opt/extensions/foo. Basically exactly the way rqlite's Dockerfile works, except basing off of rqlite's image instead of alpine. Pretty straightforward.

Then there's no need for a custom path, because everything, whether stock or custom, is under /opt/extensions. So the custom entrypoint script built into the chart would find everything in the extensions list under there.

@otoolep
Copy link
Member Author

otoolep commented Aug 17, 2024

Might extensions take custom configuration at some point?

From looking at the SQLite Extension loading C API, the only possible configuration option is an entry point (which has a sensible default). Right now I don't support it. But that entry point seems like the only possible config.

Of course assuming rqlite did eventually support multiple paths using this colon-delimited approach.

I was actually thinking of expanding -extensions-path to take comma-delimited (or colon-delimited) paths -- in other words it should be enhanced to take multiple objects. Wouldn't be a breaking change either so easy to add, and would avoid the need to copy the extensions during Docker launch. Maybe wait to make Helm changes until I get that in?

(By the way, what are Helm best practises for adding flags, but ensuring someone is running a minimum version of, in this case rqlite, the software so the flags are supported by the application?"

@jtackaberry
Copy link
Collaborator

But that entry point seems like the only possible config.

Probably not worth going out of our way to make this configurable then.

I was actually thinking of expanding -extensions-path to take comma-delimited (or colon-delimited) paths

Colon as a delimiter in paths is more idiomatic, so I'd go with that.

Maybe wait to make Helm changes until I get that in?

Probably better the other way. Once rqlite supports multiple extension paths then I can do the ideal integration as pre-rqlite-launch scripting to setup -extensions-path. Actually, if the default entrypoint script you manage also provided support for a custom path string (CUSTOM_EXTENSIONS_PATH or some such) which is concatenated with the generated path string (based on SQLITE_EXTENSIONS) the chart wouldn't have much extra work to do except simply to translate the extensions to the appropriate env vars.

(By the way, what are Helm best practises for adding flags, but ensuring someone is running a minimum version of, in this case rqlite, the software so the flags are supported by the application?"

It's technically possible using JSON Schemas for validating chart values, which Helm supports for this kind of thing, but I'm not sure if it'd be really worth it in this particular case, since the when the chart adds the extensions value it would also bump the default rqlite version at the same time. So it'd require the user to explicitly override that to an older version and explicitly define extensions in the chart values.

@otoolep
Copy link
Member Author

otoolep commented Aug 18, 2024

Colon as a delimiter in paths is more idiomatic, so I'd go with that.

A colon is a file path element in Windows though, commas are not. So wouldn't colons be an issue on Windows?

@jtackaberry
Copy link
Collaborator

A colon is a file path element in Windows though, commas are not. So wouldn't colons be an issue on Windows?

Fair enough. I'm always surprised to learn that people run anything like rqlite on Windows. :)

Windows uses semicolons to separate components in the path env var, but then that's awkward on *nix, so I guess commas are the best compromise.

@otoolep
Copy link
Member Author

otoolep commented Aug 20, 2024

@jtackaberry -- I'm proposing the following for the Docker container:

  • SQLITE_EXTENSIONS - existing environment variable which takes a list of space-delimited extensions to enable.
  • CUSTOM_SQLITE_EXTENSIONS_PATH - new environment variable which can also be set, is a set of space-delimited paths. Choosing space so it's the same as SQLITE_EXTENSIONS, no other reason.

Now users can instead explicitly pass -extensions-path to the Docker container, and it will pick that up (but any paths they pass using the command-line flag must make sense from inside the container). I'm proposing that folks must choose one way or the other, as it's too tricky (and prone to bugs) to try to parse the command line flags, merge them with any paths set in the Environment variables, etc. Unless you think it's actually fairly easy and I'm missing something?

@otoolep
Copy link
Member Author

otoolep commented Aug 20, 2024

(Previous comment edited heavily)

@otoolep
Copy link
Member Author

otoolep commented Aug 20, 2024

So an example invocation would be like this:

docker run -p4001:4001 \
   -e SQLITE_EXTENSIONS='sqlean misc'\
   -e CUSTOM_SQLITE_EXTENSIONS_PATH='/rqlite/file/my_ext' \
   rqlite/rqlite

which would end launching inside the container:

/bin/rqlited \
  -extensions-path=/opt/extensions/sqlean,/opt/extensions/misc,/rqlite/file/my_ext \
  data

@jtackaberry
Copy link
Collaborator

Space-delimited for the env vars but comma-delimited for the command line arguments -- I fear that violates the Principle of Least Astonishment, but I suppose you don't want to change SQLITE_EXTENSIONS given it's in a release now. You could move to commas entirely, but quietly support spaces in SQLITE_EXTENSIONS for backward compatibility purposes.

docker run -p4001:4001 \
-e SQLITE_EXTENSIONS='sqlean misc' \
-e CUSTOM_SQLITE_EXTENSIONS_PATH='/rqlite/file/my_ext' \
rqlite/rqlite

Would extensions located in custom extensions paths need to be included in SQLITE_EXTENSIONS, or are they automatically enabled? (I think they should need to be explicitly included in SQLITE_EXTENSIONS personally.)

I'm proposing that folks must choose one way or the other, as it's too tricky (and prone to bugs) to try to parse the command line flags, merge them with any paths set in the Environment variables, etc. Unless you think it's actually fairly easy and I'm missing something?

This seems fine to me. Typically command line arguments override env vars which override config files (the latter not being relevant in this case).

This should be pretty straightforward to add to the chart. I should also add support for custom volume mounts as well, in case users want to put extensions on a persistent volume instead of baking their own custom container.

@otoolep
Copy link
Member Author

otoolep commented Aug 21, 2024

Yeah, you're right. The spaces-vs-commas thing is not good. Let me see if I can do what you suggest.

@otoolep
Copy link
Member Author

otoolep commented Aug 22, 2024

I should also add support for custom volume mounts as well, in case users want to put extensions on a persistent volume instead of baking their own custom container.

Yeah, I think allowing folks to avoid building their own image is important (unless they like that kind of thing) -- thanks.

@otoolep
Copy link
Member Author

otoolep commented Aug 23, 2024

v8.29.0 has been released. I decided to go with the following:

  • SQLITE_EXTENSIONS: comma-delimited set of built-extensions to enable (space-delimited is silently supported). This variable only concerns built-in extensions
  • CUSTOM_SQLITE_EXTENSIONS_PATH - comma-delimited paths to extensions to also enable. You don't need to reference them in SQLITE_EXTENSIONS.

Example invocation:

docker run -p4001:4001 \
-e SQLITE_EXTENSIONS='sqlean,misc' \
-e CUSTOM_SQLITE_EXTENSIONS_PATH='/rqlite/file/my_ext' \
rqlite/rqlite

@jtackaberry
Copy link
Collaborator

Thanks! Will take a look at incorporating this into the chart over the next few days.

@otoolep
Copy link
Member Author

otoolep commented Aug 26, 2024

Cool, @jtackaberry -- bump to version v8.29.1 as v8.29.0 panicked with the vfsstat extension loaded, and certain queries were run.

@jtackaberry
Copy link
Collaborator

jtackaberry commented Aug 31, 2024

Life zigged when I was expecting it to zag. I hope to begin work on Monday but it may not happen until later in the week.

@otoolep
Copy link
Member Author

otoolep commented Aug 31, 2024

No worries, you should also set the default version to the latest release. v8.29.1 in particular introduced a fix for a panic.

@jtackaberry
Copy link
Collaborator

Yep, I always test $LATEST whenever I do a chart release :)

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

No branches or pull requests

2 participants