feat: allow overwriting deployment updateStrategy and configuration parameters#293
feat: allow overwriting deployment updateStrategy and configuration parameters#293hebestreit wants to merge 1 commit intoopenfga:mainfrom
Conversation
|
|
WalkthroughThe pull request adds configurable Kubernetes deployment update strategy support to the OpenFGA Helm chart. Three related files are modified to introduce a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable sequence diagrams in the walkthrough.Disable the |
hebestreit
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
charts/openfga/values.schema.json (1)
33-47: Consider adding validation constraints to the schema.The schema could be more specific to provide better validation and documentation:
- The
typefield could use an enum to restrict values to valid Kubernetes strategy types- The
rollingUpdateobject could define expected properties (maxSurge, maxUnavailable) with their types🔍 Enhanced schema validation
"updateStrategy": { "type": "object", "properties": { "type": { "type": "string", "description": "Strategy type", - "default": "RollingUpdate" + "default": "RollingUpdate", + "enum": ["RollingUpdate", "Recreate"] }, "rollingUpdate": { "type": "object", "description": "Rolling update configuration parameters", - "default": {} + "default": {}, + "properties": { + "maxSurge": { + "oneOf": [ + {"type": "integer", "minimum": 0}, + {"type": "string", "pattern": "^[0-9]+%$"} + ], + "description": "Maximum number of pods that can be created over the desired number" + }, + "maxUnavailable": { + "oneOf": [ + {"type": "integer", "minimum": 0}, + {"type": "string", "pattern": "^[0-9]+%$"} + ], + "description": "Maximum number of pods that can be unavailable during the update" + } + } } } }Note: While this enhances validation, Kubernetes itself will validate these fields, so this is a nice-to-have improvement rather than a critical issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/openfga/values.schema.json` around lines 33 - 47, The updateStrategy schema is too loose: tighten validation by making updateStrategy.type an enum limited to valid Kubernetes strategy values (e.g., "RollingUpdate" and "Recreate") and define rollingUpdate as an object with explicit properties maxSurge and maxUnavailable (specify their types as string or integer as appropriate and add sensible defaults or patterns), ensuring the schema uses those property names under updateStrategy.rollingUpdate and marks them optional/required per your chart expectations so consumers get better validation/documentation.charts/openfga/values.yaml (2)
3-5: Add documentation comments for the new field.Other configuration fields in this file include
@paramstyle documentation comments explaining their purpose and usage (e.g., lines 63-69 for probe configuration). TheupdateStrategyfield should follow the same pattern to help users understand how to configure it to address the PostgreSQL connection exhaustion issue mentioned in the PR objectives.📝 Suggested documentation
+## `@param` updateStrategy.type Deployment update strategy type (RollingUpdate or Recreate) +## `@param` updateStrategy.rollingUpdate Rolling update configuration parameters. Set maxSurge=0 and maxUnavailable=1 to prevent extra pods during updates. +## updateStrategy: type: RollingUpdate rollingUpdate: {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/openfga/values.yaml` around lines 3 - 5, The new Kubernetes deployment field updateStrategy (with nested type: RollingUpdate and rollingUpdate) lacks the `@param-style` documentation present for other config fields; add short `@param` comments above updateStrategy describing its purpose, valid values (e.g., RollingUpdate, Recreate), how to configure rollingUpdate knobs (e.g., maxUnavailable/maxSurge) and mention why adjusting it can mitigate PostgreSQL connection exhaustion (staggering pod restarts), mirroring the style used for probe configuration so users know how to tune it.
5-5: Consider documenting that users must configure rollingUpdate parameters to address connection exhaustion.The default
rollingUpdate: {}uses Kubernetes defaults (typically maxSurge=25%, maxUnavailable=25%), which will still create extra pods during updates. To solve the PostgreSQL connection exhaustion problem described in the PR objectives, users need to explicitly set values like:updateStrategy: type: RollingUpdate rollingUpdate: maxSurge: 0 maxUnavailable: 1Consider adding a comment or example in values.yaml showing this configuration pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/openfga/values.yaml` at line 5, Add a short explanatory comment and an example `updateStrategy` block to values.yaml near the existing `rollingUpdate: {}` so operators know to set explicit rollingUpdate parameters to avoid PostgreSQL connection exhaustion; reference the `updateStrategy` and `rollingUpdate` keys and show the recommended pattern (e.g., `type: RollingUpdate` with `rollingUpdate.maxSurge: 0` and `rollingUpdate.maxUnavailable: 1`) so users can copy it into their override values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@charts/openfga/values.schema.json`:
- Around line 33-47: The updateStrategy schema is too loose: tighten validation
by making updateStrategy.type an enum limited to valid Kubernetes strategy
values (e.g., "RollingUpdate" and "Recreate") and define rollingUpdate as an
object with explicit properties maxSurge and maxUnavailable (specify their types
as string or integer as appropriate and add sensible defaults or patterns),
ensuring the schema uses those property names under updateStrategy.rollingUpdate
and marks them optional/required per your chart expectations so consumers get
better validation/documentation.
In `@charts/openfga/values.yaml`:
- Around line 3-5: The new Kubernetes deployment field updateStrategy (with
nested type: RollingUpdate and rollingUpdate) lacks the `@param-style`
documentation present for other config fields; add short `@param` comments above
updateStrategy describing its purpose, valid values (e.g., RollingUpdate,
Recreate), how to configure rollingUpdate knobs (e.g., maxUnavailable/maxSurge)
and mention why adjusting it can mitigate PostgreSQL connection exhaustion
(staggering pod restarts), mirroring the style used for probe configuration so
users know how to tune it.
- Line 5: Add a short explanatory comment and an example `updateStrategy` block
to values.yaml near the existing `rollingUpdate: {}` so operators know to set
explicit rollingUpdate parameters to avoid PostgreSQL connection exhaustion;
reference the `updateStrategy` and `rollingUpdate` keys and show the recommended
pattern (e.g., `type: RollingUpdate` with `rollingUpdate.maxSurge: 0` and
`rollingUpdate.maxUnavailable: 1`) so users can copy it into their override
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b78a1553-200f-4d87-9f1f-dedc1cfb21c1
📒 Files selected for processing (3)
charts/openfga/templates/deployment.yamlcharts/openfga/values.schema.jsoncharts/openfga/values.yaml
There was a problem hiding this comment.
Pull request overview
This PR adds Helm chart support for configuring the Kubernetes Deployment spec.strategy so operators can tune rolling upgrade behavior (e.g., maxSurge=0) to avoid temporarily creating extra pods and exhausting shared resources like database connections.
Changes:
- Introduces a new
updateStrategyvalues entry with a RollingUpdate default. - Adds
updateStrategyto the chart JSON schema for values validation/discovery. - Renders the configured
updateStrategyinto the Deployment template.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
charts/openfga/values.yaml |
Adds a new updateStrategy values block with defaults. |
charts/openfga/values.schema.json |
Adds schema entries for updateStrategy. |
charts/openfga/templates/deployment.yaml |
Injects spec.strategy from .Values.updateStrategy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| {{- if .Values.updateStrategy }} | ||
| strategy: {{- toYaml .Values.updateStrategy | nindent 4 }} |
| "updateStrategy": { | ||
| "type": "object", | ||
| "properties": { | ||
| "type": { | ||
| "type": "string", | ||
| "description": "Strategy type", | ||
| "default": "RollingUpdate" | ||
| }, | ||
| "rollingUpdate": { | ||
| "type": "object", | ||
| "description": "Rolling update configuration parameters", | ||
| "default": {} | ||
| } | ||
| } | ||
| }, |
| @@ -1,5 +1,9 @@ | |||
| replicaCount: 3 | |||
|
|
|||
There was a problem hiding this comment.
@hebestreit - I think this PR isn't really addressing the root cause here, your DB doesn't have enough connections, sounds like you are right up against your limit. There is already a setting where you can set maxOpenConns. I suggest reducing the amount of connections, you could even use pgbouncer or similar to do connection pooling if you don't want to make any changes.
|
@emilic thanks for your quick reply. Reducing the maxOpenConns solves the problem but then we can't utilize all available database connections. In this setup the database is a db.t4g.small which has 190 max connections and we're running 3 OpenFGA instances. We followed the recommendations listed in the Running in Production guide to fine tune the server database settings. OPENFGA_DATASTORE_MAX_OPEN_CONNS
190 / 3 = ~63 (round to 60) OPENFGA_DATASTORE_MIN_IDLE_CONNS
60 * 0.75 = 45 OPENFGA_DATASTORE_MIN_OPEN_CONNS
Needs to set same as ConfigurationWe end up with below configuration: replicaCount: 3
extraEnvVars:
- name: OPENFGA_DATASTORE_MAX_OPEN_CONNS
value: "60"
- name: OPENFGA_DATASTORE_MIN_IDLE_CONNS
value: "45"
- name: OPENFGA_DATASTORE_MIN_OPEN_CONNS
value: "45" |
|
@emilic is there anything we need to configure differently or can we merge this PR? |
Description
What problem is being solved?
We configured
OPENFGA_DATASTORE_MIN_OPEN_CONNS=45and distributed the setting to all three replicas. When restarting the Deployment or doing a rolling upgrade, a fourth pod is started and fails with below error since there are no database connections available anymore.How is it being solved?
Allow the user to set the Deployment strategy and additional configuration parameters like:
The implementation can be tested by running below commands.
Overwrite
helm template openfga openfga --set updateStrategy.rollingUpdate.maxSurge=0 --set updateStrategy.rollingUpdate.maxUnavailable=1 | grep -5 RollingUpdateDefault behavior
helm template openfga openfga | grep -5 RollingUpdateWhat changes are made to solve it?
Updated the
deployment.yamland allowed to overwrite the configuration viavalues.yaml.References
Review Checklist
mainSummary by CodeRabbit
New Features