Skip to content

pulls out all 9.1 configs by immutable or modifiable#152

Draft
stockholmux wants to merge 1 commit into
valkey-io:mainfrom
stockholmux:config-list
Draft

pulls out all 9.1 configs by immutable or modifiable#152
stockholmux wants to merge 1 commit into
valkey-io:mainfrom
stockholmux:config-list

Conversation

@stockholmux

Copy link
Copy Markdown
Member

Related #141

Summary

This PR contains a list of configs that are immutable or modifiable. Requested in the meeting on 4/24.

Features / Behaviour Changes

n/a

Implementation

No idea if the path is remotely correct, feel free to suggest the correct location. The data was pulled out from the Valkey source config.c via a regular expression.

Limitations

n/a

Testing

n/a

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message explains what changed and why
  • n/a Tests are added or updated.
  • n/a Documentation files are updated.
  • n/a I have run pre-commit locally (pre-commit run --all-files or hooks on commit)

Signed-off-by: Kyle J. Davis <kyle.davis@percona.com>
@utdrmac

utdrmac commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

What do you think of resources/ and move the readiness/liveness scripts, plus this yaml in there?

@stockholmux

Copy link
Copy Markdown
Member Author

@utdrmac resources is fine by me. What do you think about the version scoping by path? E.g. /resources/9.1/config-list.yaml

I can't decide if that is needed. In theory, each version could have more/less or even change at majors.

@daanvinken

Copy link
Copy Markdown
Contributor

Can you share the command we ran to get these?


Some entries look wrong for k8s operator context, e.g. port, bind, dir, replicaof are listed as modifiable, but should trhe operator let users change these during runtime? I think some of these may be limited by the nature of Kubernetes.

Do we need a 3rd catagory like "modifiable but operator-managed"?
We may want to solve this at a different layer, just calling it out :)

@stockholmux

Copy link
Copy Markdown
Member Author

my colleague (@martinrvisser) gave them to me in a google sheet, I just converted to YAML. I'll see if he can supply the regex.

I don't think we have to be parallel perfectly to the server because, yeah, the ones you call out shouldn't be. Maybe we just move them to whatever category makes sense for the operator.

@melancholictheory

Copy link
Copy Markdown

this is a handy starting point. a vote on your version-scoping question, plus two things from doing the same classification in our operator.

on version-scoping the path (your reply to utdrmac): yes, i'd scope it per version. the modifiable set really does move between versions, this 9.1 list already has keys that don't exist on older servers (tls-auto-reload-interval is 9.1, shutdown-on-sigterm is 9.0), and valkey hard-fails on an unknown directive, so a flat cross-version list mislabels or breaks the moment you point it at the wrong server. we version-gate our config rendering off the image tag for exactly this. resources//config-list.yaml is the right shape.

the bigger thing, building on what daanvinken flagged: this captures one axis but the operator needs two. modifiable_configs here is "does CONFIG SET work on the server" (the MODIFIABLE flag in config.c). that's necessary but not sufficient for "should the operator apply this live from a user's spec". scanning the modifiable list, a big chunk of it is operator-owned: dir, bind, port, save, all the tls--file plus tls-port, cluster-announce-, replica-announce-ip, requirepass / primaryauth / primaryuser. those are all server-modifiable, but if a user sets them they either break routing/TLS/auth or just get clobbered on the next reconcile. so the live-apply allowlist wants to be the intersection: serverModifiable AND user-settable. might be worth two flags per key here rather than a single immutable/modifiable split.

cluster-announce-* is the sharp one for us. we learned the hard way that letting those drift breaks data routing in cluster mode, so they stay operator-owned no matter what config.c says about them.

last thing: a regex over config.c is a fine bridge but it'll drift, and it misses configs registered in non-standard ways. cheap guard is to boot the target-version server in CI and diff this yaml against the live CONFIG GET * set plus the modifiable flag, so the table can't silently rot per release. longer term CONFIG INFO (valkey/valkey#3050, 10.0) gives this natively, so the static list is really a bridge until then.

happy to share our split if it helps.

@melancholictheory

Copy link
Copy Markdown

following on the version-scoping question above: the static list answers "what's modifiable on version X" as data, but the operator still has to know which version it's actually talking to before it can use that. there's no version detection in the operator today, so i wanted to float the runtime side, since a few in-flight things want it (this list, the live-config allowlist, shutdown-on-sigterm which is 9.0+, tls-auto-reload-interval which is 9.1+).

the split that worked for us is by when the decision gets made:

  • runtime decisions (e.g. is this config CONFIG SET-able on this server) can read the version straight from INFO server, which getNodeState already pulls into node.Info (valkey_version / redis_version). so it's basically free wherever the operator already holds a connection, same as it reads slave_repl_offset and friends.

  • boot-time rendering (what goes into valkey.conf before any pod exists) only has the image tag to go on, since nothing is running yet. parse the tag to a version, and when it isn't parseable (:latest, a digest, a custom tag) render conservatively: drop the version-gated directives rather than risk the server rejecting an unknown one at boot.

so maybe one small helper, something like valkeyVersionAtLeast, with the INFO-server value as the source of truth and the image-tag parse as the boot-time fallback. happy to put up a PR for it if the shape sounds right; it would let the 9.0/9.1 directives gate cleanly instead of each one rolling its own check.

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