feat: add minReadySeconds and deploymentStrategy passthrough - #327
feat: add minReadySeconds and deploymentStrategy passthrough#327rwo-trackunit wants to merge 3 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds configurable ChangesDeployment rollout configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant DeploymentTemplate
participant KubernetesDeployment
HelmValues->>DeploymentTemplate: provide rollout values
DeploymentTemplate->>KubernetesDeployment: render minReadySeconds and strategy
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
5703796 to
70aad35
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
The Deployment currently always uses the Kubernetes default rollout behavior. Because OpenFGA pods typically become Ready within seconds, a rolling update replaces every replica almost simultaneously, and all pods start with cold in-memory caches (check query cache, iterator cache) at the same time - causing a datastore load spike right after each deployment. Exposing spec.minReadySeconds lets operators space out pod replacement so each new pod serves (and warms its cache with) live traffic before the next one is replaced. Exposing spec.strategy (deploymentStrategy) lets operators tune maxSurge/maxUnavailable or use Recreate. Both values are optional and omitted by default, so rendered manifests are unchanged for existing users.
70aad35 to
8b577d2
Compare
|
Tangentially related to #293, but not quite the same. |
There was a problem hiding this comment.
Pull request overview
This PR adds configurable Kubernetes Deployment rollout controls to the openfga Helm chart by exposing spec.minReadySeconds and spec.strategy as values, enabling operators to slow down rollouts and tune surge/unavailable behavior to avoid cache-cold load spikes after deployments.
Changes:
- Add
minReadySeconds(optional, supports explicit0) anddeploymentStrategypassthrough values. - Render
spec.minReadySecondsandspec.strategyintemplates/deployment.yamlonly when configured. - Add helm-unittest coverage for omission, zero/positive
minReadySeconds, and RollingUpdate/Recreate strategy passthrough; bump chart version.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| charts/openfga/values.yaml | Adds new top-level chart values for rollout controls. |
| charts/openfga/values.schema.json | Documents and constrains the new values via JSON schema. |
| charts/openfga/templates/deployment.yaml | Conditionally renders spec.minReadySeconds and spec.strategy into the Deployment. |
| charts/openfga/tests/deployment_strategy_test.yaml | Adds unit tests covering rendering/omission behavior and passthrough semantics. |
| charts/openfga/Chart.yaml | Bumps chart version to 0.3.11. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Helm merges maps recursively, so an override values file setting
deploymentStrategy: {} cannot unset a previously-defined strategy.
Allowing null lets operators explicitly clear the value so
spec.strategy is omitted; the template's 'with' guard already treats
nil as unset.
Description
The chart's Deployment currently always uses the Kubernetes default rollout behavior and exposes no knobs to control it. Because OpenFGA pods typically become Ready within seconds, a rolling update replaces every replica almost simultaneously — so all pods start with cold in-memory caches (check query cache, iterator cache) at the same moment, causing a datastore load spike right after every deployment.
This PR exposes two standard Deployment fields as chart values:
minReadySeconds(default: unset) — rendered asspec.minReadySecondswhen set. Lets operators space out pod replacement so each new pod serves (and warms its cache with) live traffic before the next pod is replaced, keeping at most one cold-cache pod in rotation at a time.deploymentStrategy(default:{}) — passed through verbatim asspec.strategywhen set. Lets operators tunemaxSurge/maxUnavailableor useRecreate.Both are optional and omitted by default, so rendered manifests are unchanged for existing users.
Changes
templates/deployment.yaml: renderminReadySeconds(guarded withne ... nilso an explicit0renders, matching the existing falsy-numeric handling) andstrategyvalues.yaml+values.schema.json: document the new values (minReadySecondsasinteger|null,deploymentStrategyas object)tests/deployment_strategy_test.yaml: helm-unittest coverage — unset (omitted),0, positive value, RollingUpdate passthrough, RecreateChart.yaml: bump version to 0.3.11🤖 Generated with Claude Code
Real life implications
For our usage, we're seeing connection storm at the time of deployment. To be fair, the same could happen under extreme heavy load, where every single call misses the cache. So this is supporting cost-saving, where we rightsize the deployment based on actual usage, but that "actual usage" becomes worst case during deployments.
deploymentStrategyis only added as due-diligence, for future possible deployment tweaks. Let me know if we want to omit that for now.Summary by CodeRabbit
New Features
Documentation
Tests