Skip to content

Add Valkey addon support#720

Merged
phinze merged 2 commits intomainfrom
phinze/mir-897-add-valkey-addon-support
Apr 2, 2026
Merged

Add Valkey addon support#720
phinze merged 2 commits intomainfrom
phinze/mir-897-add-valkey-addon-support

Conversation

@phinze
Copy link
Copy Markdown
Contributor

@phinze phinze commented Apr 1, 2026

We've had PostgreSQL and MySQL as built-in addons for a while, and Valkey was the obvious next one to add. It follows the same dbsaga provider pattern but ends up noticeably simpler — Valkey doesn't need a SQL driver since provisioning just starts a container and hands back connection info. No per-app users or databases to create.

Only the dedicated ("small") variant ships in this PR. We considered a shared variant, but Valkey doesn't have a clean multitenant story the way databases do — there are no per-app databases or users, so a shared instance would silently give all apps access to each other's keys. We can add a shared variant later with ACL-based key isolation if there's demand.

Both VALKEY_* and REDIS_* env vars are emitted so apps using standard Redis client libraries (which auto-discover REDIS_URL) work without any extra config. A future miren-redis addon will own the REDIS_* namespace properly, but for now the overlap is harmless and saves users from aliasing.

One small framework change: CreateSandboxPoolSpec now has a Command field so addons can pass arguments to the container entrypoint. Valkey needs this for --requirepass and --save flags since the official image doesn't read those from env vars the way MySQL/PostgreSQL do.

RDB persistence is enabled with a disk volume at /data, so data survives container restarts. Verified end-to-end in the dev environment: provisioning, env var injection, health checks on port 6379, and full deprovision cleanup all work.

Closes MIR-897

Valkey is a Redis-compatible in-memory key-value store. This adds it
as the third built-in addon alongside PostgreSQL and MySQL, following
the same provider pattern but simpler — no SQL driver needed since
Valkey provisioning just starts a container and returns connection
info.

Only the dedicated ("small") variant ships for now. Unlike databases,
Valkey doesn't have per-app users or databases, so a shared variant
would give all apps access to each other's keys. We can revisit with
ACL-based isolation if there's demand.

The provider emits both VALKEY_* and REDIS_* env vars so apps using
standard Redis client libraries work out of the box. Auth is handled
via --requirepass on the container command, and both variants include
RDB persistence with a disk volume at /data.

Small framework addition: CreateSandboxPoolSpec now accepts a Command
field, wired through to the container spec. Existing addons don't set
it so no behavior change there.

Closes MIR-897
@phinze phinze requested a review from a team as a code owner April 1, 2026 22:23
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 1, 2026

📝 Walkthrough

Walkthrough

Adds a Valkey addon provider and plans, implements provisioning and deprovisioning via saga workflows (dedicated variant), and registers the provider in the coordinator. Introduces two new schema kinds: ValkeyServer and ValkeyDedicatedData. Extends sandbox pool creation spec with a Command field. Adds provider code (Provision/Deprovision, helpers), dedicated saga implementations and tests, plan metadata and tests, blackbox test for addon lifecycle, and updates generated schema registration to include the new entities.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/addon/valkey/dedicated.go`:
- Around line 120-142: The CreateSandboxPool call embeds the password in the
Command string (in CreateSandboxPoolSpec) which exposes it; instead remove the
password from Command (leave just "valkey-server --save 60 1" or equivalent) and
set the CreateSandboxPoolSpec.Env slice to include the Valkey flags with the
password (e.g. an entry like "VALKEY_EXTRA_FLAGS=--requirepass <password> --save
60 1"); update the Command/Env usage in pkg/addon/valkey/dedicated.go around the
CreateSandboxPool invocation (referencing CreateSandboxPool,
CreateSandboxPoolSpec, Command and Env fields) so the password is passed via Env
rather than in the command line.

In `@pkg/addon/valkey/provider.go`:
- Around line 33-37: The host:port construction in buildValkeyURL uses
fmt.Sprintf("%s:%d", host, port) which fails for IPv6; replace that with
net.JoinHostPort(host, strconv.Itoa(port)) and add the "net" and "strconv"
imports (or use fmt.Sprintf only for port-to-string if strconv already present)
so the url.URL{ Host: net.JoinHostPort(...) } handles IPv6 correctly; apply the
same change pattern where similar fmt.Sprintf host:port usage appears (e.g.,
MySQL/Postgres helper functions).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e4eb5544-7d5f-4861-9104-0d1e701eca7d

📥 Commits

Reviewing files that changed from the base of the PR and between 84d49fb and 04b366d.

📒 Files selected for processing (10)
  • api/addon/addon_v1alpha/schema.gen.go
  • api/addon/schema.yml
  • blackbox/addon_test.go
  • components/coordinate/coordinate.go
  • pkg/addon/framework.go
  • pkg/addon/valkey/dedicated.go
  • pkg/addon/valkey/dedicated_test.go
  • pkg/addon/valkey/plans.go
  • pkg/addon/valkey/plans_test.go
  • pkg/addon/valkey/provider.go

Use net.JoinHostPort for URL host construction so IPv6 addresses
are handled correctly. Move the requirepass flag from the container
command into the VALKEY_EXTRA_FLAGS env var, which is the mechanism
the official Valkey image provides for passing server flags. Also
drop the shared variant — Valkey doesn't have per-app isolation, so
shared would silently expose all apps' data to each other.
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
pkg/addon/valkey/dedicated.go (1)

283-301: Consider consolidating the two GetById calls into one.

Lines 287 and 292 make two separate database calls for the same entity ID to retrieve ValkeyServer and Metadata. This could be consolidated into a single call if the entity framework supports retrieving multiple aspects at once, reducing database round-trips during deprovisioning.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/addon/valkey/dedicated.go` around lines 283 - 301, Consolidate the two
separate fw.EC.GetById calls in LookupDedicatedServer into a single retrieval to
avoid duplicate round-trips: call fw.EC.GetById once for in.DedicatedServerID
and populate both addon_v1alpha.ValkeyServer and core_v1alpha.Metadata in the
same call (or fetch the raw entity once and unmarshal/extract into server and
meta), then return the same LookupDedicatedServerOut; update
LookupDedicatedServer to use a single fw.EC.GetById invocation (or equivalent
multi-aspect API) instead of separate calls for addon_v1alpha.ValkeyServer and
core_v1alpha.Metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/addon/valkey/dedicated.go`:
- Around line 283-301: Consolidate the two separate fw.EC.GetById calls in
LookupDedicatedServer into a single retrieval to avoid duplicate round-trips:
call fw.EC.GetById once for in.DedicatedServerID and populate both
addon_v1alpha.ValkeyServer and core_v1alpha.Metadata in the same call (or fetch
the raw entity once and unmarshal/extract into server and meta), then return the
same LookupDedicatedServerOut; update LookupDedicatedServer to use a single
fw.EC.GetById invocation (or equivalent multi-aspect API) instead of separate
calls for addon_v1alpha.ValkeyServer and core_v1alpha.Metadata.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0d42fc04-2781-4241-9e2f-96df364204af

📥 Commits

Reviewing files that changed from the base of the PR and between 04b366d and 022c3f8.

📒 Files selected for processing (2)
  • pkg/addon/valkey/dedicated.go
  • pkg/addon/valkey/provider.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/addon/valkey/provider.go

@phinze phinze merged commit 1e4d7f4 into main Apr 2, 2026
11 checks passed
@phinze phinze deleted the phinze/mir-897-add-valkey-addon-support branch April 2, 2026 19:39
phinze added a commit that referenced this pull request Apr 2, 2026
Follows the same dedicated-instance pattern established by Valkey in
#720, with a few simplifications that reflect how memcached works:
no persistent storage (it's an in-memory cache), no authentication
(network-isolated on-cluster), and a straightforward set of env vars
(MEMCACHE_URL, MEMCACHE_HOST, MEMCACHE_PORT — none sensitive).

The "small" variant runs memcached:1.6 with a 64 MB memory limit.
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.

2 participants