-
Notifications
You must be signed in to change notification settings - Fork 23
(feat): Extra Valkey config #44
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
base: main
Are you sure you want to change the base?
Changes from all commits
a05d5d6
7241a35
05af544
8919932
f066a91
cc614b7
88cf17a
02808a3
59bcd2e
3a1709a
8dce649
26e809e
f487a3c
7780156
3db1fd2
cc6b210
28712d8
8a75a87
a2c1818
3736d67
925e406
932a3dd
ac3ee7a
074afe4
8b16ccf
790854f
7077bb5
d85bbd2
2d9d3c7
224a39f
245c64f
91070a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2639,10 +2639,6 @@ spec: | |
| More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ | ||
| type: object | ||
| type: object | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jdheyburn FYI on this change. I renamed it to "better align"(?) with the other changes since the configmap has both config and the scripts. |
||
| scriptsConfigMapName: | ||
| description: ScriptsConfigMapName specifies the name of the ConfigMap | ||
| that contains the scripts for the ValkeyNode. | ||
| type: string | ||
| tolerations: | ||
| description: Tolerations defines the pod tolerations. | ||
| items: | ||
|
|
@@ -2689,6 +2685,11 @@ spec: | |
| file. When set, mounts a users-acl volume from this Secret so the | ||
| container can load aclfile /config/users/users.acl. | ||
| type: string | ||
| usersConfigMapName: | ||
| description: |- | ||
| UsersConfigMapName specifies the name of the ConfigMap that contains the | ||
| scripts, and Valkey config for the ValkeyNode. | ||
| type: string | ||
| workloadType: | ||
| default: StatefulSet | ||
| description: |- | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,224 @@ | ||
| /* | ||
| Copyright 2025 Valkey Contributors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package controller | ||
|
|
||
| import ( | ||
| "context" | ||
| "crypto/sha256" | ||
| "embed" | ||
| "fmt" | ||
| "maps" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| corev1 "k8s.io/api/core/v1" | ||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" | ||
| logf "sigs.k8s.io/controller-runtime/pkg/log" | ||
| valkeyiov1alpha1 "valkey.io/valkey-operator/api/v1alpha1" | ||
| ) | ||
|
|
||
| const ( | ||
| scriptsHashKey = "valkey.io/script-hash" | ||
| configHashKey = "valkey.io/config-hash" | ||
| configFileKey = "valkey.conf" | ||
|
|
||
| readinessScriptKey = "readiness-check.sh" | ||
| livenessScriptKey = "liveness-check.sh" | ||
|
|
||
| // This hash should be updated whenever the contents of either script changes, which would | ||
| // coincide with operator version bump. | ||
| // $ cat internal/controller/scripts/{liveness-check.sh,readiness-check.sh} | sha256sum | ||
| scriptsHash = "8531132f52ac311772dfcb45c107c34ab05e719a0df644cc332512277b564346" | ||
|
|
||
| // Average-ish length of Valkey parameter + value | ||
| averageParameterLength = 20 | ||
| ) | ||
|
|
||
| //go:embed scripts/* | ||
| var scripts embed.FS | ||
|
|
||
| func getConfigMapName(clusterName string) string { | ||
| return clusterName + "-config" | ||
| } | ||
|
|
||
| // Return a base config of parameters that users shouldn't be able to override | ||
| func getBaseConfig() string { | ||
| return `# Base operator config | ||
| cluster-enabled yes | ||
| protected-mode no | ||
| cluster-node-timeout 2000 | ||
| aclfile /config/users/users.acl | ||
| ` | ||
| } | ||
|
|
||
| func getUserConfig(ctx context.Context, cluster *valkeyiov1alpha1.ValkeyCluster) string { | ||
|
|
||
| specConfig := cluster.Spec.Config | ||
|
|
||
| // Exit early if nothing | ||
| if len(specConfig) == 0 { | ||
| return "" | ||
| } | ||
|
|
||
| log := logf.FromContext(ctx) | ||
|
|
||
| // Build the config | ||
| var configBuilder strings.Builder | ||
| configBuilder.Grow(len(specConfig) * averageParameterLength) | ||
| writeConfigLine(&configBuilder, "#", "Extra Config") | ||
|
|
||
| // Sort the config keys to keep consistent processing order | ||
| sortedKeys := slices.Sorted(maps.Keys(specConfig)) | ||
|
|
||
| for _, param := range sortedKeys { | ||
|
|
||
| if slices.Contains(valkeyiov1alpha1.NonUserOverrideConfigParameters, param) { | ||
| log.Error(nil, "Prohibited valkey server config", "parameter", param) | ||
| continue | ||
| } | ||
|
|
||
| writeConfigLine(&configBuilder, param, specConfig[param]) | ||
| } | ||
|
|
||
| return configBuilder.String() | ||
| } | ||
|
|
||
| // 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 { | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we rename this file to configmap.go
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point |
||
| log := logf.FromContext(ctx) | ||
|
|
||
| // Embed readiness check script | ||
| readiness, err := scripts.ReadFile("scripts/readiness-check.sh") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if this is nitpicking, but does it make sense to have scripts in a configmap named I would argue to keep
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is your suggestion to move the scripts to their own configMap? I have no objection with that.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine to leave as is. We can refactor later on |
||
| if err != nil { | ||
| return fmt.Errorf("reading embedded readiness-check.sh: %w", err) | ||
| } | ||
|
|
||
| // Embed liveness check script | ||
| liveness, err := scripts.ReadFile("scripts/liveness-check.sh") | ||
| if err != nil { | ||
| return fmt.Errorf("reading embedded liveness-check.sh: %w", err) | ||
| } | ||
|
|
||
| // Get base config | ||
| var newConfigBuilder strings.Builder | ||
| newConfigBuilder.WriteString(getBaseConfig()) | ||
|
|
||
| // User-provided config from spec | ||
| newConfigBuilder.WriteString(getUserConfig(ctx, cluster)) | ||
|
|
||
| // Final string version of the config | ||
| newServerConfig := newConfigBuilder.String() | ||
|
|
||
| // Calculate hash of constructed configMap contents (ie: updated scripts, changed/added parameters) | ||
| newServerConfigHash := fmt.Sprintf("%x", sha256.Sum256([]byte(newServerConfig))) | ||
|
|
||
| // Look for, and fetch existing configMap for this cluster | ||
| serverConfigMapName := getConfigMapName(cluster.Name) | ||
| serverConfigMap := &corev1.ConfigMap{} | ||
| if err := r.Get(ctx, types.NamespacedName{ | ||
| Name: serverConfigMapName, | ||
| Namespace: cluster.Namespace, | ||
| }, serverConfigMap); err != nil { | ||
| if !apierrors.IsNotFound(err) { | ||
| log.Error(err, "failed to fetch server configmap") | ||
| return err | ||
| } | ||
|
|
||
| // ConfigMap not found; This happens on cluster init | ||
| log.V(2).Info("creating server configMap", "name", serverConfigMapName) | ||
|
|
||
| // Create configMap object with contents | ||
| serverConfigMap.ObjectMeta = metav1.ObjectMeta{ | ||
| Name: serverConfigMapName, | ||
| Namespace: cluster.Namespace, | ||
| Labels: labels(cluster), | ||
| Annotations: map[string]string{ | ||
| configHashKey: newServerConfigHash, | ||
| scriptsHashKey: scriptsHash, | ||
| }, | ||
| } | ||
| serverConfigMap.Data = map[string]string{ | ||
| readinessScriptKey: string(readiness), | ||
| livenessScriptKey: string(liveness), | ||
| configFileKey: newServerConfig, | ||
| } | ||
|
|
||
| // Register ownership of the configMap | ||
| if err := controllerutil.SetControllerReference(cluster, serverConfigMap, r.Scheme); err != nil { | ||
| log.Error(err, "Failed to grab ownership of server configMap") | ||
| r.Recorder.Eventf(cluster, nil, corev1.EventTypeWarning, "ConfigMapCreationFailed", "UpsertConfigMap", "Failed to grab ownership of server configMap: %v", err) | ||
| return err | ||
| } | ||
|
|
||
| // Create the configMap | ||
| if err := r.Create(ctx, serverConfigMap); err != nil { | ||
| log.Error(err, "Failed to create server configMap") | ||
| r.Recorder.Eventf(cluster, nil, corev1.EventTypeWarning, "ConfigMapCreationFailed", "UpsertConfigMap", "Failed to create server configMap: %v", err) | ||
| return err | ||
| } | ||
|
|
||
| r.Recorder.Eventf(cluster, nil, corev1.EventTypeNormal, "ConfigMapCreated", "UpsertConfigMap", "Created server configMap") | ||
|
|
||
| // All good; new configMap with contents created | ||
| return nil | ||
| } | ||
|
|
||
| // ConfigMap exists | ||
|
|
||
| // Compare scripts hash in existing configMap to const value in operator; update scripts contents if different | ||
| updatedScripts := upsertAnnotation(serverConfigMap, scriptsHashKey, scriptsHash) | ||
| if updatedScripts { | ||
| log.V(1).Info("updated readiness, and liveness scripts") | ||
| serverConfigMap.Data[readinessScriptKey] = string(readiness) | ||
| serverConfigMap.Data[livenessScriptKey] = string(liveness) | ||
| } | ||
|
|
||
| // If the generated config contents hash (from above) matches the hash of the current | ||
| // config contents, and we did not update the scripts contents, exit early | ||
| if !updatedScripts && !upsertAnnotation(serverConfigMap, configHashKey, newServerConfigHash) { | ||
| log.V(1).Info("server config unchanged") | ||
| return nil | ||
| } | ||
|
|
||
| // Update the configMap with the generated config contents | ||
| serverConfigMap.Data[configFileKey] = newServerConfig | ||
|
|
||
| // Update | ||
| if err := r.Update(ctx, serverConfigMap); err != nil { | ||
| log.Error(err, "Failed to update server configMap") | ||
| r.Recorder.Eventf(cluster, nil, corev1.EventTypeWarning, "ConfigMapUpdateFailed", "UpsertConfigMap", "Failed to update server configMap: %v", err) | ||
| return err | ||
| } | ||
|
|
||
| r.Recorder.Eventf(cluster, nil, corev1.EventTypeNormal, "ConfigMapUpdated", "UpsertConfigMap", "Synchronized server configMap") | ||
|
|
||
| // All is good. configMap was updated with new contents. | ||
| return nil | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont think updated configmap will be auto mounted in the existing deployments during update. Did u test this
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seem to update the configfile in the pods when I test it on But, I now see that we are not using
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree that we should omit live applying changes to the config for another PR |
||
| } | ||
|
|
||
| // Helper function to write a config line in the form of "parameter value\n" to a strings.Builder | ||
| func writeConfigLine(builder *strings.Builder, name, value string) { | ||
| builder.WriteString(name) | ||
| builder.WriteString(" ") | ||
| builder.WriteString(value) | ||
| builder.WriteString("\n") | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.