fix: stop updating workloads on every reconcile by owning API-server defaults (#317)

Fixes #315.

## Problem

`ensureStatefulSet` / `ensureDeployment` assign the whole desired spec
inside their `CreateOrUpdate` mutate functions, while the builders leave
every API-server-defaulted field unset. Each reconcile clobbers the
stored defaults back to zero values, `CreateOrUpdate` sees a diff, and
the operator issues an Update — every pass, forever, with
`metadata.generation` stuck. Observed impact: ~1–2k
`statefulsets.update` Admin Activity audit events/hour on a
3-shard/6-node cluster; on GKE these land in the non-excludable
`_Required` bucket (details in #315).

This implements the **narrow fix** discussed in the issue thread (own
the defaults in the builders), as suggested there — small and
backportable; a Server-Side Apply migration can follow separately.

## Changes

**`buildValkeyNodePodTemplateSpec` / `buildContainersDef`** — set the
pod-template fields the API server would default, so the built template
already equals the stored one:
- pod: `restartPolicy`, `dnsPolicy`, `schedulerName`,
`terminationGracePeriodSeconds` (30 when unset), empty `securityContext`
when none is configured, volume `defaultMode`s
- containers (applied after `mergePatchContainers`, so user container
patches are normalized the same way the API server would):
`imagePullPolicy` (tag-aware, mirroring API-server logic),
`terminationMessagePath`/`Policy`, port `protocol`, env
`fieldRef.apiVersion`

**`buildValkeyNodeStatefulSet`** — `podManagementPolicy`,
`updateStrategy` (RollingUpdate, partition 0), `revisionHistoryLimit`,
`persistentVolumeClaimRetentionPolicy` (Retain/Retain; note: requires
the `StatefulSetAutoDeletePVC` gate, on by default since 1.27, GA 1.32 —
on older clusters with the gate off the API server drops the field and
this one field would resume churning).

**`buildValkeyNodeDeployment`** — `revisionHistoryLimit`,
`progressDeadlineSeconds`.

**`upsertService`** — the rebuilt `ports` slice now sets `protocol` and
`targetPort`, which made the headless Service churn the same way.

The PDB reconcile already mutates field-wise with values that round-trip
cleanly, so it needed no change.

## Tests

- New envtest regression test: reconcile a ValkeyNode, capture the
StatefulSet `resourceVersion`, reconcile 3 more times, assert it is
unchanged. Fails on `main`, passes here (envtest runs a real API server,
so real defaulting is exercised).
- `TestBuildValkeyNodePodTemplateSpec_PodSecurityContext_NilIsNoop`
updated: omitting the field now yields the empty `securityContext` the
API server defaults to — semantically identical (no security settings
applied), and required for the no-op property.
- `make test` green (75/75 specs + unit tests, envtest k8s 1.35).

## Caveat

This is intentionally the treadmill-y narrow fix: a future Kubernetes
version defaulting a new field would reintroduce churn for that field
until added here. The durable fix is SSA with generated
ApplyConfigurations, per the discussion in #315.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Signed-off-by: Matan David <matan.david@eon.io>
Signed-off-by: Joseph Heyburn <34041368+jdheyburn@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Joseph Heyburn <34041368+jdheyburn@users.noreply.github.com>
Co-authored-by: Joseph Heyburn <jdheyburn@gmail.com>

Author: 3 people

internal/controller/valkeycluster_controller.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/client-go/tools/events"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -489,7 +490,15 @@ func (r *ValkeyClusterReconciler) upsertService(ctx context.Context, cluster *va
 			svc.Spec.ClusterIP = "None"
 		}
 		svc.Spec.Selector = map[string]string{LabelCluster: cluster.Name}
-		svc.Spec.Ports = []corev1.ServicePort{{Name: appName, Port: DefaultPort}}
+		// Protocol and TargetPort are API-server defaults; set them explicitly
+		// so the rebuilt slice deep-equals the stored one and CreateOrUpdate
+		// stops updating the Service on every reconcile (#315).
+		svc.Spec.Ports = []corev1.ServicePort{{
+			Name:       appName,
+			Port:       DefaultPort,
+			Protocol:   corev1.ProtocolTCP,
+			TargetPort: intstr.FromInt32(DefaultPort),
+		}}
 		return controllerutil.SetControllerReference(cluster, svc, r.Scheme)
 	})
 	if err != nil {

internal/controller/valkeycluster_controller_test.go

@@ -755,6 +755,37 @@ var _ = Describe("EventRecorder", func() {
 			Expect(events).To(ContainElement(ContainSubstring("Created headless Service")))
 		})
 
+		It("should not update the headless Service on repeated upserts", func() {
+			cluster := &valkeyiov1alpha1.ValkeyCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "svc-noop-cluster",
+					Namespace: "default",
+				},
+				Spec: valkeyiov1alpha1.ValkeyClusterSpec{
+					Shards:   3,
+					Replicas: 1,
+				},
+			}
+			Expect(k8sClient.Create(ctx, cluster)).To(Succeed())
+			defer func() { _ = k8sClient.Delete(ctx, cluster) }()
+
+			Expect(r.upsertService(ctx, cluster)).To(Succeed())
+			svc := &corev1.Service{}
+			svcKey := types.NamespacedName{Name: headlessServiceName(cluster.Name), Namespace: cluster.Namespace}
+			Expect(k8sClient.Get(ctx, svcKey, svc)).To(Succeed())
+			createdResourceVersion := svc.ResourceVersion
+
+			// The API server defaults the port's protocol and targetPort when
+			// the Service is stored; the rebuilt ports slice must already carry
+			// them or every upsert Updates the Service (#315).
+			for range 3 {
+				Expect(r.upsertService(ctx, cluster)).To(Succeed())
+			}
+			Expect(k8sClient.Get(ctx, svcKey, svc)).To(Succeed())
+			Expect(svc.ResourceVersion).To(Equal(createdResourceVersion),
+				"an upsert with no changes must not write the Service")
+		})
+
 		It("should emit ConfigMapCreated event on successful configmap creation", func() {
 			cluster := &valkeyiov1alpha1.ValkeyCluster{
 				ObjectMeta: metav1.ObjectMeta{

internal/controller/valkeycluster_pdb_test.go

@@ -101,6 +101,25 @@ var _ = Describe("reconcilePodDisruptionBudget", func() {
 
 			Expect(k8sClient.Get(ctx, pdbKey, pdb)).To(Succeed())
 		})
+
+		It("does not update the PDB on repeated reconciles when nothing changed", func() {
+			Expect(reconciler.reconcilePodDisruptionBudget(ctx, cluster)).To(Succeed())
+
+			pdb := &policyv1.PodDisruptionBudget{}
+			Expect(k8sClient.Get(ctx, pdbKey, pdb)).To(Succeed())
+			createdResourceVersion := pdb.ResourceVersion
+
+			// The PDB mutate assigns fields individually (it never replaces the
+			// whole spec), so anything the API server defaults on the stored
+			// object — e.g. unhealthyPodEvictionPolicy on newer clusters — must
+			// survive and repeated reconciles must be no-ops (#315).
+			for range 3 {
+				Expect(reconciler.reconcilePodDisruptionBudget(ctx, cluster)).To(Succeed())
+			}
+			Expect(k8sClient.Get(ctx, pdbKey, pdb)).To(Succeed())
+			Expect(pdb.ResourceVersion).To(Equal(createdResourceVersion),
+				"a reconcile with no changes must not write the PDB")
+		})
 	})
 
 	Context("when PodDisruptionBudget mode is Cluster", func() {

internal/controller/valkeynode_controller_test.go

@@ -192,6 +192,37 @@ var _ = Describe("ValkeyNode Controller", func() {
 			Expect(sts.Spec.Template.Labels).To(HaveKeyWithValue("app.kubernetes.io/component", "valkey-node"))
 		})
 
+		It("should not update the StatefulSet on reconciles when nothing changed", func() {
+			r := &ValkeyNodeReconciler{
+				Client:   k8sClient,
+				Scheme:   k8sClient.Scheme(),
+				Recorder: events.NewFakeRecorder(100),
+			}
+
+			By("creating the StatefulSet on first reconcile")
+			_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName})
+			Expect(err).NotTo(HaveOccurred())
+
+			sts := &appsv1.StatefulSet{}
+			Expect(k8sClient.Get(ctx, statefulSetName, sts)).To(Succeed())
+			createdResourceVersion := sts.ResourceVersion
+
+			// The API server fills in defaults (podManagementPolicy,
+			// updateStrategy, imagePullPolicy, ...) when the StatefulSet is
+			// stored. The desired object built on the next pass must already
+			// carry those values, otherwise CreateOrUpdate sees a diff and
+			// issues an Update on every reconcile (#315).
+			By("reconciling again without any spec change")
+			for range 3 {
+				_, err = r.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName})
+				Expect(err).NotTo(HaveOccurred())
+			}
+
+			Expect(k8sClient.Get(ctx, statefulSetName, sts)).To(Succeed())
+			Expect(sts.ResourceVersion).To(Equal(createdResourceVersion),
+				"a reconcile with no changes must not write the StatefulSet")
+		})
+
 		It("should set Ready=false with PodNotReady condition when no pod exists", func() {
 			By("Reconciling the created resource")
 			r := &ValkeyNodeReconciler{

internal/controller/valkeynode_resources.go

@@ -19,6 +19,7 @@ package controller
 import (
 	"encoding/json"
 	"fmt"
+	"strings"
 
 	valkeyiov1alpha1 "github.com/valkey-io/valkey-operator/api/v1alpha1"
 	appsv1 "k8s.io/api/apps/v1"
@@ -307,7 +308,76 @@ func buildContainersDef(node *valkeyiov1alpha1.ValkeyNode) ([]corev1.Container,
 		containers = append(containers, generateMetricsExporterContainerDef(node.Spec.Exporter, node.Labels[LabelCluster], node.Spec.TLS))
 	}
 
-	return mergePatchContainers(containers, node.Spec.Containers)
+	merged, err := mergePatchContainers(containers, node.Spec.Containers)
+	if err != nil {
+		return nil, err
+	}
+	applyContainerAPIDefaults(merged)
+	return merged, nil
+}
+
+// applyContainerAPIDefaults fills the container fields the API server would
+// otherwise default (imagePullPolicy, terminationMessagePath/Policy, port
+// protocol, env fieldRef apiVersion). The workload reconcilers assign the
+// whole desired spec in their CreateOrUpdate mutate functions, so a field
+// left unset here is clobbered to its zero value on every pass and the
+// operator updates the workload on every reconcile even though nothing
+// changed (#315). Runs after mergePatchContainers so user-supplied container
+// patches are normalized the same way the API server would normalize them.
+func applyContainerAPIDefaults(containers []corev1.Container) {
+	for i := range containers {
+		c := &containers[i]
+		if c.ImagePullPolicy == "" {
+			c.ImagePullPolicy = defaultImagePullPolicy(c.Image)
+		}
+		if c.TerminationMessagePath == "" {
+			c.TerminationMessagePath = corev1.TerminationMessagePathDefault
+		}
+		if c.TerminationMessagePolicy == "" {
+			c.TerminationMessagePolicy = corev1.TerminationMessageReadFile
+		}
+		for j := range c.Ports {
+			if c.Ports[j].Protocol == "" {
+				c.Ports[j].Protocol = corev1.ProtocolTCP
+			}
+		}
+		for j := range c.Env {
+			if vf := c.Env[j].ValueFrom; vf != nil && vf.FieldRef != nil && vf.FieldRef.APIVersion == "" {
+				vf.FieldRef.APIVersion = "v1"
+			}
+		}
+	}
+}
+
+// defaultImagePullPolicy mirrors the API server's imagePullPolicy defaulting
+// (SetDefaults_Container + parsers.ParseImageName): the effective tag is the
+// explicit tag when present — even alongside a digest — "latest" when the
+// reference has neither tag nor digest, and empty for a digest-only reference.
+// The policy is Always exactly when the effective tag is "latest",
+// IfNotPresent otherwise.
+func defaultImagePullPolicy(image string) corev1.PullPolicy {
+	name := image
+	if i := strings.IndexByte(name, '@'); i >= 0 {
+		name = name[:i] // strip the digest; an explicit tag before it still counts
+	}
+	tag := ""
+	// ':' only introduces a tag after the last '/' (a registry port such as
+	// "reg:5000/img" is not a tag).
+	if i := strings.LastIndexByte(name, ':'); i > strings.LastIndexByte(name, '/') {
+		tag = name[i+1:]
+	}
+	switch {
+	case tag == "latest":
+		return corev1.PullAlways
+	case tag != "":
+		return corev1.PullIfNotPresent
+	case len(name) < len(image):
+		// Digest-only reference (no tag): IfNotPresent.
+		return corev1.PullIfNotPresent
+	default:
+		// No tag, no digest: the reference normalizes to :latest.
+		return corev1.PullAlways
+	}
 }
 
 // buildValkeyNodePodTemplateSpec constructs a PodTemplateSpec for a single
@@ -335,6 +405,15 @@ func buildValkeyNodePodTemplateSpec(node *valkeyiov1alpha1.ValkeyNode, labels ma
 		TopologySpreadConstraints:     node.Spec.TopologySpreadConstraints,
 		SecurityContext:               node.Spec.PodSecurityContext,
 		TerminationGracePeriodSeconds: node.Spec.TerminationGracePeriodSeconds,
+		// Fields below are set to the API server's defaults. The workload
+		// reconcilers assign the whole desired spec in their CreateOrUpdate
+		// mutate functions, so any field left unset here gets clobbered back
+		// to its zero value on every pass, the API server re-defaults it, and
+		// the operator issues an Update on every reconcile even though nothing
+		// changed (#315).
+		RestartPolicy: corev1.RestartPolicyAlways,
+		DNSPolicy:     corev1.DNSClusterFirst,
+		SchedulerName: corev1.DefaultSchedulerName,
 		Volumes: []corev1.Volume{
 			{
 				Name: "scripts",
@@ -404,6 +483,23 @@ func buildValkeyNodePodTemplateSpec(node *valkeyiov1alpha1.ValkeyNode, labels ma
 	}
 	podSpec.Volumes = append(podSpec.Volumes, dataVolume)
 
+	// Mirror the remaining API-server defaults (see the comment on the struct
+	// literal above): a nil here is not "unset" once stored, it is a diff.
+	if podSpec.SecurityContext == nil {
+		podSpec.SecurityContext = &corev1.PodSecurityContext{}
+	}
+	if podSpec.TerminationGracePeriodSeconds == nil {
+		podSpec.TerminationGracePeriodSeconds = func(i int64) *int64 { return &i }(corev1.DefaultTerminationGracePeriodSeconds)
+	}
+	for i := range podSpec.Volumes {
+		if cm := podSpec.Volumes[i].ConfigMap; cm != nil && cm.DefaultMode == nil {
+			cm.DefaultMode = func(i int32) *int32 { return &i }(corev1.ConfigMapVolumeSourceDefaultMode)
+		}
+		if sec := podSpec.Volumes[i].Secret; sec != nil && sec.DefaultMode == nil {
+			sec.DefaultMode = func(i int32) *int32 { return &i }(corev1.SecretVolumeSourceDefaultMode)
+		}
+	}
+
 	return corev1.PodTemplateSpec{
 		ObjectMeta: metav1.ObjectMeta{
 			Labels: labels,
@@ -435,6 +531,11 @@ func buildValkeyNodeDeployment(node *valkeyiov1alpha1.ValkeyNode) (*appsv1.Deplo
 				MatchLabels: labels,
 			},
 			Template: tmpl,
+			// API-server defaults, set explicitly so the desired spec equals
+			// the stored one and ensureDeployment's wholesale spec assignment
+			// stops producing an Update on every reconcile (#315).
+			RevisionHistoryLimit:    func(i int32) *int32 { return &i }(10),
+			ProgressDeadlineSeconds: func(i int32) *int32 { return &i }(600),
 		},
 	}, nil
 }
@@ -461,6 +562,21 @@ func buildValkeyNodeStatefulSet(node *valkeyiov1alpha1.ValkeyNode) (*appsv1.Stat
 				MatchLabels: labels,
 			},
 			Template: tmpl,
+			// API-server defaults, set explicitly so the desired spec equals
+			// the stored one and ensureStatefulSet's wholesale spec assignment
+			// stops producing an Update on every reconcile (#315).
+			PodManagementPolicy: appsv1.OrderedReadyPodManagement,
+			UpdateStrategy: appsv1.StatefulSetUpdateStrategy{
+				Type: appsv1.RollingUpdateStatefulSetStrategyType,
+				RollingUpdate: &appsv1.RollingUpdateStatefulSetStrategy{
+					Partition: func(i int32) *int32 { return &i }(0),
+				},
+			},
+			RevisionHistoryLimit: func(i int32) *int32 { return &i }(10),
+			PersistentVolumeClaimRetentionPolicy: &appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy{
+				WhenDeleted: appsv1.RetainPersistentVolumeClaimRetentionPolicyType,
+				WhenScaled:  appsv1.RetainPersistentVolumeClaimRetentionPolicyType,
+			},
 		},
 	}, nil
 }

internal/controller/valkeynode_resources_test.go

@@ -1017,13 +1017,42 @@ func TestBuildValkeyNodePodTemplateSpec_PodSecurityContext_Passthrough(t *testin
 }
 
 // TestBuildValkeyNodePodTemplateSpec_PodSecurityContext_NilIsNoop confirms
-// backward compatibility: omitting the field results in no pod-level
-// SecurityContext (existing CRs unchanged).
+// backward compatibility: omitting the field applies no security settings.
+// The built template carries the empty SecurityContext the API server would
+// default a stored pod template to — semantically identical to nil, and
+// required so an unchanged reconcile stays a no-op (#315).
 func TestBuildValkeyNodePodTemplateSpec_PodSecurityContext_NilIsNoop(t *testing.T) {
 	node := newTestValkeyNode("mynode", "test-ns")
 
 	pts, err := buildValkeyNodePodTemplateSpec(node, valkeyNodeLabels(node))
 	require.NoError(t, err)
 
-	assert.Nil(t, pts.Spec.SecurityContext, "omitting PodSecurityContext must leave pod-level SecurityContext nil")
+	assert.Equal(t, &corev1.PodSecurityContext{}, pts.Spec.SecurityContext,
+		"omitting PodSecurityContext must produce the empty SecurityContext the API server defaults to")
+}
+
+// TestDefaultImagePullPolicy pins the mirror of the API server's defaulting
+// rule branch by branch — a silent divergence on any of these makes that
+// container churn on every reconcile (#315). Rule: the effective tag is the
+// explicit tag when present (even alongside a digest), "latest" when the
+// reference has neither tag nor digest, empty for digest-only; policy is
+// Always exactly when the effective tag is "latest".
+func TestDefaultImagePullPolicy(t *testing.T) {
+	cases := []struct {
+		image string
+		want  corev1.PullPolicy
+	}{
+		{"valkey/valkey:8.0.1", corev1.PullIfNotPresent},                  // plain tag
+		{"valkey/valkey:latest", corev1.PullAlways},                       // latest tag
+		{"valkey/valkey", corev1.PullAlways},                              // no tag, no digest
+		{"valkey/valkey@sha256:deadbeef", corev1.PullIfNotPresent},        // digest only
+		{"valkey/valkey:8.0.1@sha256:deadbeef", corev1.PullIfNotPresent},  // tag + digest
+		{"valkey/valkey:latest@sha256:deadbeef", corev1.PullAlways},       // latest tag wins over digest
+		{"registry:5000/valkey", corev1.PullAlways},                       // registry port is not a tag
+		{"registry:5000/valkey:8.0.1", corev1.PullIfNotPresent},           // registry port + tag
+		{"registry:5000/valkey@sha256:deadbeef", corev1.PullIfNotPresent}, // registry port + digest
+	}
+	for _, tc := range cases {
+		assert.Equal(t, tc.want, defaultImagePullPolicy(tc.image), "image %q", tc.image)
+	}
 }