(feat): Extra Valkey config by utdrmac · Pull Request #44 · valkey-io/valkey-operator

utdrmac · 2026-01-09T23:12:05Z

This feature request implements the ability for users to supply their own configuration for Valkey as an inline stanza when deploying a new cluster. The user configuration is appended to the required base configuration, which is created as a configMap. Since the operator manages the configMap, any changes to the config are reconciled. Additionally, users cannot override the required cluster settings.

This PR implements the ability for users to supply additional configuration for Valkey. Any provided parameters are appended to default configuration required by the operator Signed-off-by: utdrmac <matthew.boehm@percona.com>

Signed-off-by: utdrmac <matthew.boehm@percona.com>

config/samples/v1alpha1_valkeycluster.yaml

internal/controller/valkeycluster_controller.go

Signed-off-by: utdrmac <matthew.boehm@percona.com>

api/v1alpha1/valkeycluster_types.go

Signed-off-by: utdrmac <matthew.boehm@percona.com>

sandeepkunusoth

LGTM

sandeepkunusoth

lint errors and e2e errors

Signed-off-by: utdrmac <matthew.boehm@percona.com>

utdrmac · 2026-01-10T03:12:22Z

ugh. why didn't lint/fmt/vet catch those?

Signed-off-by: utdrmac <matthew.boehm@percona.com>

jdheyburn

Minor comments, keen to hear opinions, otherwise LGTM

api/v1alpha1/valkeycluster_types.go

internal/controller/valkeycluster_controller.go

Signed-off-by: utdrmac <matthew.boehm@percona.com>

utdrmac · 2026-01-12T21:48:26Z

e2e still fails due to #43

api/v1alpha1/valkeycluster_types.go

sandeepkunusoth · 2026-01-12T22:04:35Z

e2e still fails due to #43

no this is different issue than 43. its failing because of events limitation. maybe as u have added new emit event other events are not coming. https://github.com/valkey-io/valkey-operator/actions/runs/20936033780/job/60159472854?pr=44.

check this PR description #37. maybe you can update events check accordingly. currently we are not asserting for all events due to events rate limit.

utdrmac · 2026-01-12T22:29:42Z

I only added 1 new event which isn't triggered during the e2e. When I run e2e on my local kind cluster, I don't get that failure; I get the same failure as #43.

sandeepkunusoth · 2026-01-12T22:46:00Z

I only added 1 new event which isn't triggered during the e2e. When I run e2e on my local kind cluster, I don't get that failure; I get the same failure as #43.

no i don't think its related. you can check the issue description failure test case and logs are different for both of these errors. i have neven seen failure of valkey creation CR test case in main branch. if in case we are seeing, we need to create different issue.

this PR is failing this test "Manager when a ValkeyCluster CR is applied [It] creates a Valkey Cluster deployment"

43 issue is with other test "Manager when a ValkeyCluster experiences degraded state [It] should detect and recover when a deployment is deleted" https://github.com/valkey-io/valkey-operator/actions/runs/20806446577/job/59761668293

quick fix: we may need to remove assertions of some of the events.

valkey-operator/test/e2e/e2e_test.go

Line 405 in 44d674b

    
           g.Expect(output).To(ContainSubstring("ClusterMeet"), "ClusterMeet event should appear in describe")

bjosv

LGTM

bjosv · 2026-01-15T12:01:13Z

Rebasing (or add merge commit of main) should fix the e2e test now.

Signed-off-by: utdrmac <matthew.boehm@percona.com>

internal/controller/valkeycluster_controller.go

Signed-off-by: utdrmac <matthew.boehm@percona.com>

utdrmac · 2026-01-22T16:20:15Z

@bjosv The test failed on some trivial text matching. I removed it because the two lines above are checking the same string.

internal/controller/config.go

utdrmac

Everything is resolved. @sandeepkunusoth github says you've requested changes but I can't find anything unresolved when looking at the 'Files changed' tab.

sandeepkunusoth · 2026-01-22T23:30:42Z

internal/controller/config.go

+	r.Recorder.Eventf(cluster, corev1.EventTypeNormal, "ConfigMapUpdated", "Synchronized server configMap")
+
+	// All is good. Server configMap will be auto-mounted in the deployment
+	return nil


i dont think updated configmap will be auto mounted in the existing deployments during update. Did u test this

It seem to update the configfile in the pods when I test it on ~~kind~~ minikube now, but it takes some time before the file is getting updated.

But, I now see that we are not using SubPath in the VolumeMounts on containers..
We probably should (?) since both /scripts and /configs contains all files from the configmap.
The problem then will be that the volume mount will not receive updates when the ConfigMap changes though..

Updating the configmap is one thing, but the Valkey instances will not use these updates anyway in the current form. Is that what you where thinking on @sandeepkunusoth ?
Maybe the update part in this PR could be moved to its own PR, which could include the pod restart/config reload depending on how we decide.

Agree that we should omit live applying changes to the config for another PR

internal/controller/config.go

dismm

sandeepkunusoth · 2026-01-23T17:29:55Z

internal/controller/config.go

+// Create or update a default valkey.conf
+// If additional config is provided, append to the default map
+func (r *ValkeyClusterReconciler) upsertConfigMap(ctx context.Context, cluster *valkeyiov1alpha1.ValkeyCluster) error {
+


can we rename this file to configmap.go

bjosv · 2026-01-29T12:14:39Z

internal/controller/config.go

+	r.Recorder.Eventf(cluster, corev1.EventTypeNormal, "ConfigMapUpdated", "Synchronized server configMap")
+
+	// All is good. Server configMap will be auto-mounted in the deployment
+	return nil


It seem to update the configfile in the pods when I test it on ~~kind~~ minikube now, but it takes some time before the file is getting updated.

But, I now see that we are not using SubPath in the VolumeMounts on containers..
We probably should (?) since both /scripts and /configs contains all files from the configmap.
The problem then will be that the volume mount will not receive updates when the ConfigMap changes though..

bjosv · 2026-01-29T12:15:48Z

internal/controller/config.go

+// Create or update a default valkey.conf
+// If additional config is provided, append to the default map
+func (r *ValkeyClusterReconciler) upsertConfigMap(ctx context.Context, cluster *valkeyiov1alpha1.ValkeyCluster) error {
+


bjosv · 2026-01-29T12:26:04Z

internal/controller/config.go

+	// Calculate hash of new config parameters
+	newServerConfigHash := fmt.Sprintf("%x", sha256.Sum256([]byte(serverConfig)))
+
+	// Was the configMap changed? This is an invalid action, as users should modify


This is a bit hard to follow with a lot of flags (maybe I need more coffee..)
It possibly simpler to just create the configmap at the start of this function, including its annotation, and then return thereafter.
This part would only handle the cases when the configmap already exists in some form.
I think that would remove the need of needCreateConfigMap and origConfigModified

bjosv · 2026-01-29T12:33:39Z

internal/controller/config.go

+	r.Recorder.Eventf(cluster, corev1.EventTypeNormal, "ConfigMapUpdated", "Synchronized server configMap")
+
+	// All is good. Server configMap will be auto-mounted in the deployment
+	return nil


Updating the configmap is one thing, but the Valkey instances will not use these updates anyway in the current form. Is that what you where thinking on @sandeepkunusoth ?
Maybe the update part in this PR could be moved to its own PR, which could include the pod restart/config reload depending on how we decide.

internal/controller/utils.go

Signed-off-by: utdrmac <matthew.boehm@percona.com>

jdheyburn · 2026-02-11T16:36:18Z

internal/controller/config.go

+	hashConfigContents := sha256.New()
+
+	// Embed readiness check script
+	readiness, err := scripts.ReadFile("scripts/readiness-check.sh")


I don't know if this is nitpicking, but does it make sense to have scripts in a configmap named mycluster-config? That to me indicates that the ConfigMap contains only configurations.

I would argue to keep -config out of the resource name and just keep it to the name of the cluster.

Is your suggestion to move the scripts to their own configMap? I have no objection with that.

I think it's fine to leave as is. We can refactor later on

jdheyburn · 2026-02-11T16:38:34Z

internal/controller/config.go

+	if len(specConfig) > 0 {
+
+		// Sort the config keys to keep consistent processing order
+		sortedKeys := slices.Sorted(maps.Keys(specConfig))
+
+		// Build the config
+		serverConfig += "\n# Extra config\n"
+		for _, k := range sortedKeys {
+
+			if slices.Contains(valkeyiov1alpha1.NonUserOverrideConfigParameters, k) {
+				log.Error(nil, "Prohibited valkey server config", "parameter", k)
+				r.Recorder.Eventf(cluster, nil, corev1.EventTypeWarning, "ConfigMapUpdateFailed", "UpsertConfigMap", "Prohibited config: %v", k)
+				continue
+			}
+
+			serverConfig += k + " " + specConfig[k] + "\n"
+		}
+	}


Could this be extracted into its own function? Something like:

if len(cluster.Spec.ValkeySpec.Configuration) > 0 { appendSpecConfig(&serverConfig, cluster.Spec.ValkeySpec.Configuration) }

I guess it could. What's the benefit though of having the same code but in another function? Doesn't that introduce additional code jumps?

The benefit is readability - ideally we want to keep isolated units of work behind functions that are named well so that when reading the entirety of upsertConfigMap it is easy to follow along and not get bogged down on the low-level implementation of building the extra config :)

jdheyburn · 2026-02-11T16:40:25Z

internal/controller/config.go

+	r.Recorder.Eventf(cluster, corev1.EventTypeNormal, "ConfigMapUpdated", "Synchronized server configMap")
+
+	// All is good. Server configMap will be auto-mounted in the deployment
+	return nil


Agree that we should omit live applying changes to the config for another PR

utdrmac added 3 commits January 9, 2026 15:44

(feat): Extra Valkey Config

a05d5d6

This PR implements the ability for users to supply additional configuration for Valkey. Any provided parameters are appended to default configuration required by the operator Signed-off-by: utdrmac <matthew.boehm@percona.com>

corrected map name

7241a35

Signed-off-by: utdrmac <matthew.boehm@percona.com>

update sample cr

05af544

Signed-off-by: utdrmac <matthew.boehm@percona.com>

utdrmac mentioned this pull request Jan 9, 2026

(feat): Add support for auto-mounting extra configs #42

Closed

sandeepkunusoth reviewed Jan 9, 2026

View reviewed changes

config/samples/v1alpha1_valkeycluster.yaml Outdated Show resolved Hide resolved

internal/controller/valkeycluster_controller.go Outdated Show resolved Hide resolved

internal/controller/valkeycluster_controller.go Outdated Show resolved Hide resolved

utdrmac added 2 commits January 9, 2026 18:57

e2e fix; revert func name

8919932

Signed-off-by: utdrmac <matthew.boehm@percona.com>

config as map

f066a91

Signed-off-by: utdrmac <matthew.boehm@percona.com>

sandeepkunusoth reviewed Jan 10, 2026

View reviewed changes

api/v1alpha1/valkeycluster_types.go Show resolved Hide resolved

utdrmac added 2 commits January 9, 2026 20:34

add TODO comment, and e2e test

cc614b7

Signed-off-by: utdrmac <matthew.boehm@percona.com>

add TODO comment, and e2e test

88cf17a

Signed-off-by: utdrmac <matthew.boehm@percona.com>

sandeepkunusoth approved these changes Jan 10, 2026

View reviewed changes

sandeepkunusoth previously requested changes Jan 10, 2026

View reviewed changes

fix lint catch

02808a3

Signed-off-by: utdrmac <matthew.boehm@percona.com>

utdrmac requested a review from sandeepkunusoth January 10, 2026 03:04

e2e lint

59bcd2e

Signed-off-by: utdrmac <matthew.boehm@percona.com>

jdheyburn reviewed Jan 10, 2026

View reviewed changes

utdrmac added 2 commits January 12, 2026 15:36

merge main

3a1709a

Signed-off-by: utdrmac <matthew.boehm@percona.com>

merge upstream; expand e2e client commands

8dce649

Signed-off-by: utdrmac <matthew.boehm@percona.com>

sandeepkunusoth reviewed Jan 12, 2026

View reviewed changes

api/v1alpha1/valkeycluster_types.go Show resolved Hide resolved

bjosv previously approved these changes Jan 14, 2026

View reviewed changes

Merge branch 'main' into valkeyConfig

26e809e

Signed-off-by: utdrmac <matthew.boehm@percona.com>

sandeepkunusoth reviewed Jan 15, 2026

View reviewed changes

internal/controller/valkeycluster_controller.go Outdated Show resolved Hide resolved

utdrmac mentioned this pull request Jan 17, 2026

(feat): Support user creation on cluster init #56

Closed

refactor server config

f487a3c

Signed-off-by: utdrmac <matthew.boehm@percona.com>

utdrmac added 4 commits January 20, 2026 13:24

forgot file

7780156

Signed-off-by: utdrmac <matthew.boehm@percona.com>

verify original hash

3db1fd2

Signed-off-by: utdrmac <matthew.boehm@percona.com>

merge main

cc6b210

Signed-off-by: utdrmac <matthew.boehm@percona.com>

merge origin

28712d8

Signed-off-by: utdrmac <matthew.boehm@percona.com>

utdrmac requested a review from sandeepkunusoth January 20, 2026 21:36

utdrmac added 2 commits January 21, 2026 11:29

fix merge issue

8a75a87

Signed-off-by: utdrmac <matthew.boehm@percona.com>

test check text changed

a2c1818

Signed-off-by: utdrmac <matthew.boehm@percona.com>

sandeepkunusoth reviewed Jan 22, 2026

View reviewed changes

internal/controller/config.go Show resolved Hide resolved

utdrmac commented Jan 22, 2026

View reviewed changes

sandeepkunusoth reviewed Jan 22, 2026

View reviewed changes

internal/controller/config.go Show resolved Hide resolved

sandeepkunusoth reviewed Jan 23, 2026

View reviewed changes

sandeepkunusoth requested a review from bjosv January 23, 2026 17:35

bjosv reviewed Jan 29, 2026

View reviewed changes

utdrmac added 2 commits February 10, 2026 22:44

Merge branch 'main' into ghvalkeyconfig

3736d67

Signed-off-by: utdrmac <matthew.boehm@percona.com>

update Record events; add scripts to hash

925e406

Signed-off-by: utdrmac <matthew.boehm@percona.com>

jdheyburn reviewed Feb 11, 2026

View reviewed changes

utdrmac dismissed bjosv’s stale review via 925e406 February 19, 2026 14:44

Comments

Conversation

utdrmac commented Jan 9, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandeepkunusoth left a comment

Choose a reason for hiding this comment

Uh oh!

sandeepkunusoth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

utdrmac commented Jan 10, 2026

Uh oh!

jdheyburn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

utdrmac commented Jan 12, 2026

Uh oh!

Uh oh!

sandeepkunusoth commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

utdrmac commented Jan 12, 2026

Uh oh!

sandeepkunusoth commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjosv left a comment

Choose a reason for hiding this comment

Uh oh!

bjosv commented Jan 15, 2026

Uh oh!

Uh oh!

utdrmac commented Jan 22, 2026

Uh oh!

Uh oh!

utdrmac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjosv Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjosv Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sandeepkunusoth left a comment •

edited

Loading

sandeepkunusoth commented Jan 12, 2026 •

edited

Loading

sandeepkunusoth commented Jan 12, 2026 •

edited

Loading

utdrmac left a comment •

edited

Loading

bjosv Jan 29, 2026 •

edited

Loading

bjosv Jan 29, 2026 •

edited

Loading