Skip to content

enhancement: Use Recreate deployment strategy - #225

Merged
jdheyburn merged 1 commit into
valkey-io:mainfrom
jdheyburn:jdheyburn/enhancement/deployment-recreate-strategy
Jun 8, 2026
Merged

enhancement: Use Recreate deployment strategy#225
jdheyburn merged 1 commit into
valkey-io:mainfrom
jdheyburn:jdheyburn/enhancement/deployment-recreate-strategy

Conversation

@jdheyburn

Copy link
Copy Markdown
Collaborator

Summary

ValkeyNode is a 1:1 mapping of node to pod. By default the Deployment's updateStrategy is set to RollingUpdate which will add a new pod first before removing the old one. This can create 2 pods for a ValkeyNode at once. Recreate will remove the old pod first before creating the new one.

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
  • Tests are added or updated.
  • Documentation files are updated.
  • I have run pre-commit locally (pre-commit run --all-files or hooks on commit)

ValkeyNode is a 1:1 mapping of node to pod. By default the Deployment's
updateStrategy is set to RollingUpdate which will add a new pod first before
removing the old one. This can create 2 pods for a ValkeyNode at once.
Recreate will remove the old pod first before creating the new one.

Signed-off-by: Joseph Heyburn <jdheyburn@gmail.com>
@jdheyburn

Copy link
Copy Markdown
Collaborator Author

@greptile-apps

@greptile-apps

greptile-apps Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR sets the DeploymentStrategy to Recreate for ValkeyNode Deployments. Because each ValkeyNode maps 1:1 to a single pod, the default RollingUpdate strategy would temporarily spin up two pods (new before old is removed), which is undesirable for a single-replica, stateful node.

  • Sets appsv1.RecreateDeploymentStrategyType in buildValkeyNodeDeployment, ensuring the old pod is terminated before the new one is started during updates.
  • The existing TestBuildValkeyNodeDeployment test does not assert the new Strategy field, leaving the behaviour untested.

Confidence Score: 5/5

The change is safe to merge — it sets a well-understood Kubernetes strategy on a single-replica Deployment and has no impact on other workload types.

The addition is a single-field change on a single-replica Deployment. Recreate is the correct strategy for this 1:1 node-to-pod model, and the surrounding code is untouched. The only gap is the existing unit test not yet asserting the new strategy field.

No files require special attention beyond optionally extending TestBuildValkeyNodeDeployment in valkeynode_resources_test.go to cover the new strategy field.

Important Files Changed

Filename Overview
internal/controller/valkeynode_resources.go Adds Recreate deployment strategy to buildValkeyNodeDeployment to prevent two pods from coexisting for the same ValkeyNode during updates.

Sequence Diagram

sequenceDiagram
    participant Op as Operator
    participant K8s as Kubernetes API
    participant OldPod as Old Pod
    participant NewPod as New Pod

    Note over Op,NewPod: Before this PR (RollingUpdate)
    Op->>K8s: Update Deployment
    K8s->>NewPod: Create new pod
    NewPod-->>K8s: Running
    K8s->>OldPod: Terminate old pod
    Note over OldPod,NewPod: ⚠️ Both pods exist simultaneously

    Note over Op,NewPod: After this PR (Recreate)
    Op->>K8s: Update Deployment
    K8s->>OldPod: Terminate old pod
    OldPod-->>K8s: Terminated
    K8s->>NewPod: Create new pod
    NewPod-->>K8s: Running
    Note over NewPod: ✅ Only one pod at a time
Loading

Reviews (2): Last reviewed commit: "enhancement: Use Recreate deployment str..." | Re-trigger Greptile

@jdheyburn

jdheyburn commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator Author

@bjosv I've lost the chaos testing thread where I mentioned this before - do you think this is worth adding?

edit: found it! #215

@jdheyburn
jdheyburn marked this pull request as ready for review June 6, 2026 15:50
@bjosv

bjosv commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

@bjosv I've lost the chaos testing thread where I mentioned this before - do you think this is worth adding?

Ah, yes, this is probably better! Will just testrun this and then close PR #215.
I now understand that .spec.strategy.type should only affect how rollouts are done, so the deployment/replicaset should still be able to start a new pod to replace an unreachable pod.

@jdheyburn

Copy link
Copy Markdown
Collaborator Author

@bjosv Maybe #215 is still needed if it makes it more stable. It was a smell that Deployment would have 2 pods alive at the same time.

@jdheyburn
jdheyburn merged commit a759ded into valkey-io:main Jun 8, 2026
8 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in valkey-operator 0.2.0 Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants