feat: configurable terminationGracePeriodSeconds for graceful failover

[melancholictheory]

Closes #260

Summary

Add spec.terminationGracePeriodSeconds to ValkeyCluster (threaded through to ValkeyNode and onto the pod) so the graceful CLUSTER FAILOVER triggered on SIGTERM has time to hand the shard off to a replica before SIGKILL.

Design

Following the direction in #260:

• New TerminationGracePeriodSeconds *int64 on both ValkeyClusterSpec (user-facing) and ValkeyNodeSpec (the cluster controller threads it through to the pod). Not grouped under a pod-template surface yet, per your note that we can redesign that while still in alpha.
• When unset, the operator derives a safe value: max(30s, cluster-manual-failover-timeout / 1000 + 10s). With the defaults (5s timeout) that stays at the Kubernetes default of 30s. Raising the timeout pulls the grace period up with it.
• An explicit value is honoured as-is, so a user who wants a long grace period (for example to allow a final RDB snapshot) gets exactly what they asked for. If the value is below the recommended minimum, the operator emits a GracePeriodTooShort warning event on the ValkeyCluster instead of silently overriding it.

On the "block the apply" idea

You floated blocking the manifest apply when the value is too short. i went with respect-the-value-and-warn instead, because the recommended minimum depends on cluster-manual-failover-timeout, which lives in spec.config as a string map entry. a CEL admission rule can't cleanly parse and compare that, so a hard block would need a validating webhook. happy to add that as a follow-up if you'd rather it be a hard stop; the warning event is the lighter "inform the user" mechanism for now.

Acceptance criteria

• Warns (event + log) when terminationGracePeriodSeconds is below the recommended minimum
• [~] Reconcile-time check (CEL block deferred, see above)
• Documented in the CRD field comments and docs/valkeycluster.md

Testing

• Unit tests for the timeout / recommended / effective grace-period helpers.
• make test and make lint pass locally.

Docs note: i added a Termination grace period section, which sits next to the Graceful shutdown section from #268 once that merges.

Checklist

• 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 (ran make test and make lint instead)

[greptile-apps]

Greptile Summary

This PR adds spec.terminationGracePeriodSeconds to ValkeyCluster so that the graceful CLUSTER FAILOVER triggered on SIGTERM has enough time to hand off a shard before SIGKILL arrives. The field is threaded through from ValkeyClusterSpec → ValkeyNodeSpec → PodSpec, with a safe auto-derived default (max(30s, cluster-manual-failover-timeout + 10s)) and a one-shot warning event/condition when an explicit value is below the computed minimum.

• Grace period helpers (failoverTimeoutSeconds, recommendedGracePeriodSeconds, effectiveGracePeriodSeconds) parse the cluster-manual-failover-timeout config value and derive a safe default, covered by new unit tests.
• Warning condition (ConfigurationWarning / GracePeriodTooShort) is set idempotently on each reconcile while the value stays too short; the event fires only on the first transition into that state (guarded by meta.IsStatusConditionTrue).
• Nil-on-default trick in buildClusterValkeyNode avoids rolling all existing nodes on operator upgrade: the TerminationGracePeriodSeconds field on ValkeyNode is left nil when the resolved value equals the Kubernetes default of 30s.

Confidence Score: 5/5

Safe to merge; the grace-period logic is well-tested, the nil-on-default trick prevents an unintended rolling restart on upgrade, and the warning event correctly fires only once per state transition.

The implementation correctly threads terminationGracePeriodSeconds from ValkeyClusterSpec through to the pod, with safe auto-derivation, idempotent warning conditions, and upgrade-safe nil handling. All early-return paths in Reconcile call updateStatus before exiting so the ConfigurationWarning condition is always persisted. The unit tests cover edge cases including nil config, unparseable values, ceiling-division rounding, and the nil-on-default behaviour. No functional defects found.

No files require special attention.

Important Files Changed

Filename	Overview
internal/controller/valkeycluster_controller.go	Adds grace-period helpers and idempotent ConfigurationWarning condition; event guard correctly fires only on first transition into warning state; all early-return paths call updateStatus so the condition is always persisted.
internal/controller/grace_period_test.go	New unit tests cover all helper functions including edge cases (nil map, unparseable values, round-up, nil-on-default for upgrade safety).
internal/controller/valkeynode_resources.go	Passes TerminationGracePeriodSeconds from ValkeyNodeSpec directly to the PodSpec; straightforward field addition.
api/v1alpha1/valkeycluster_types.go	Adds TerminationGracePeriodSeconds field with kubebuilder minimum:1 validation, plus ConditionConfigurationWarning and ReasonGracePeriodTooShort constants.
api/v1alpha1/valkeynode_types.go	Adds TerminationGracePeriodSeconds to ValkeyNodeSpec with matching minimum:1 kubebuilder marker.
api/v1alpha1/zz_generated.deepcopy.go	Auto-generated deep-copy for the new *int64 fields; correct nil-guard pattern.
config/crd/bases/valkey.io_valkeyclusters.yaml	CRD schema for terminationGracePeriodSeconds with minimum:1 added; matches kubebuilder marker in the types file.
config/crd/bases/valkey.io_valkeynodes.yaml	CRD schema for ValkeyNode terminationGracePeriodSeconds with minimum:1.
docs/valkeycluster.md	Adds Termination grace period section accurately describing auto-derive logic, warning behavior, and CRD constraint.
docs/status-conditions.md	Documents the new ConfigurationWarning condition and GracePeriodTooShort reason; accurate and consistent with implementation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[ValkeyCluster spec
terminationGracePeriodSeconds] --> B{Explicit value set?}
    B -- Yes --> C{value < recommended?
failoverTimeout + 10s}
    C -- Yes --> D[Set ConfigurationWarning
condition = True]
    D --> E{Was condition
already True?}
    E -- No --> F[Emit one-shot
GracePeriodTooShort event]
    E -- Yes --> G[Skip event
already emitted]
    C -- No --> H[Remove ConfigurationWarning
condition if present]
    B -- No --> I[Derive effective value
max 30s, failoverTimeout + 10s]
    I --> H
    B -- Yes --> J[Store value verbatim
on ValkeyNode]
    I --> K{effective == 30s
Kubernetes default?}
    K -- Yes --> L[Leave ValkeyNode field nil
avoid upgrade churn]
    K -- No --> M[Set ValkeyNode field
to derived value]
    J --> N[ValkeyNodeSpec
TerminationGracePeriodSeconds]
    L --> N
    M --> N
    N --> O[PodSpec
TerminationGracePeriodSeconds]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[ValkeyCluster spec
terminationGracePeriodSeconds] --> B{Explicit value set?}
    B -- Yes --> C{value < recommended?
failoverTimeout + 10s}
    C -- Yes --> D[Set ConfigurationWarning
condition = True]
    D --> E{Was condition
already True?}
    E -- No --> F[Emit one-shot
GracePeriodTooShort event]
    E -- Yes --> G[Skip event
already emitted]
    C -- No --> H[Remove ConfigurationWarning
condition if present]
    B -- No --> I[Derive effective value
max 30s, failoverTimeout + 10s]
    I --> H
    B -- Yes --> J[Store value verbatim
on ValkeyNode]
    I --> K{effective == 30s
Kubernetes default?}
    K -- Yes --> L[Leave ValkeyNode field nil
avoid upgrade churn]
    K -- No --> M[Set ValkeyNode field
to derived value]
    J --> N[ValkeyNodeSpec
TerminationGracePeriodSeconds]
    L --> N
    M --> N
    N --> O[PodSpec
TerminationGracePeriodSeconds]

Loading

_{Reviews (6): Last reviewed commit: "fix: address review feedback on terminat..." | Re-trigger Greptile}

[greptile-apps]

T-Rex pricing update — T-Rex was free through June 2026. Effective July 1, 2026, T-Rex adds 2 credits on top of the standard 1-credit review (3 total). T-Rex settings

[sandeepkunusoth]

nit: can we call recommendedGracePeriodSeconds once here? we can create constant and reuse that? if recalculation is not required?

[melancholictheory]

good call, computed it once before the branch now. 4c16269.

[sandeepkunusoth]

what happens when previous graceperiod is 60 sec and if user wants to change graceperiod to default value 30 sec will this check cause updates to ignore silently?

[melancholictheory]

it doesn't drop the update silently: going from &60 to nil is still a spec change, so the pod does update to 30 (via the kubernetes default). but you're right that storing nil for an explicitly requested value is confusing, so i changed it to store explicit values verbatim, including 30. the field only stays nil for an unset value that resolves to the default, which is the case that keeps existing clusters from rolling on upgrade. 4c16269.

[sandeepkunusoth]

(nit) i think its better to add logs in error cases in 709 and 712 when it falls back to defaultFailoverTimeoutSeconds

[melancholictheory]

the missing-key path (709) is the common case, so a log there would fire on every reconcile for clusters that never set the timeout. a malformed value (712) also fails at valkey boot, and validating config values is tracked separately in #141, so i left the helper silent for now. happy to add a warning specifically for the unparseable case if you'd prefer it here.

[sandeepkunusoth]

ReasonConfigurationValid is not used anywhere

[melancholictheory]

removed, thanks. 4c16269.

[sandeepkunusoth]

(nit) can u update docs/status-conditions.md with this new status.

[melancholictheory]

added a ConfigurationWarning section to docs/status-conditions.md in 4c16269.

[jdheyburn]

LGTM - thanks for raising! Will let @sandeepkunusoth have another look too.

[jdheyburn]

This will be useful elsewhere, thanks!

[melancholictheory]

thanks for the review. agreed, the ConfigurationWarning condition and the recommended-minimum helper should carry over to any config-versus-resource guard down the line. happy to iterate if anything comes up downstream.

[jdheyburn]

Thank you!