[INFRA-456] - feat(plane-enterprise): expose gunicorn rotation and Celery broker envs; add PLANE_INTERNAL_API_HOST to pi-api#271
Conversation
…v vars Add GUNICORN_MAX_REQUESTS, GUNICORN_MAX_REQUESTS_JITTER, CELERY_TASK_PUBLISH_RETRY, and CELERY_BROKER_POOL_LIMIT to the app-vars ConfigMap so operators can tune worker rotation and broker pool behaviour without rebuilding the image. Defaults set to Kubernetes-friendly values (rotation disabled, publish retry enabled, pool bounded to 10) to eliminate the silent task-dispatch failures reported in issue #261. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…ERNAL_API_HOST to pi-api Add to app-vars ConfigMap (configurable via values.yaml): - GUNICORN_MAX_REQUESTS (default 0 — rotation disabled, recommended for K8s) - GUNICORN_MAX_REQUESTS_JITTER (default 0) - CELERY_TASK_PUBLISH_RETRY (default True — prevent silent task drops on AMQP reconnect) - CELERY_BROKER_POOL_LIMIT (default 10 — bound connection pool to avoid stale accumulation) Add to pi-api-vars ConfigMap (hardcoded internal cluster URL): - PLANE_INTERNAL_API_HOST — points directly to the in-cluster API service Fixes the silent CSV export failures reported in issue #261 where gunicorn worker rotation caused stale AMQP connections to silently discard published tasks. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…ETRY Align with existing boolean pattern (ternary without default) so that setting celery_task_publish_retry: false in values.yaml is respected. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe enterprise Helm chart adds configurable Gunicorn and Celery environment settings, renders them into application configuration, adds an internal API host, documents the new options, and increments the chart version. ChangesEnterprise chart configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)charts/plane-enterprise/questions.ymlTraceback (most recent call last): charts/plane-enterprise/templates/config-secrets/app-env.yamlTraceback (most recent call last): charts/plane-enterprise/values.yamlTraceback (most recent call last): 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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/plane-enterprise/README.md`:
- Around line 503-504: Update the Celery settings entries
env.celery_task_publish_retry and env.celery_broker_pool_limit in the README
configuration documentation to place them in a shared API/worker environment
section or explicitly identify both workloads as consumers. Preserve their
existing defaults and descriptions.
In `@charts/plane-enterprise/templates/config-secrets/app-env.yaml`:
- Line 82: Update the CELERY_BROKER_POOL_LIMIT template expression to preserve
an explicitly configured value of 0 instead of allowing default to replace it
with 10. Use presence-based handling such as hasKey on
.Values.env.celery_broker_pool_limit, while retaining 10 only when the key is
absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 30bc2b85-10b9-4fc4-b6b0-7c1bedd3e104
📒 Files selected for processing (6)
charts/plane-enterprise/Chart.yamlcharts/plane-enterprise/README.mdcharts/plane-enterprise/questions.ymlcharts/plane-enterprise/templates/config-secrets/app-env.yamlcharts/plane-enterprise/templates/config-secrets/pi-api-env.yamlcharts/plane-enterprise/values.yaml
| GUNICORN_MAX_REQUESTS: {{ .Values.env.gunicorn_max_requests | default 0 | quote }} | ||
| GUNICORN_MAX_REQUESTS_JITTER: {{ .Values.env.gunicorn_max_requests_jitter | default 0 | quote }} | ||
| CELERY_TASK_PUBLISH_RETRY: {{ .Values.env.celery_task_publish_retry | ternary "True" "False" | quote }} | ||
| CELERY_BROKER_POOL_LIMIT: {{ .Values.env.celery_broker_pool_limit | default 10 | quote }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
helm template plane-enterprise charts/plane-enterprise \
--set env.celery_broker_pool_limit=0 |
grep -F 'CELERY_BROKER_POOL_LIMIT: "0"'Repository: makeplane/helm-charts
Length of output: 197
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== candidate files =="
fd -a 'app-env.yaml|values.yaml|Chart.yaml' charts 2>/dev/null | sort
echo
echo "== app-env relevant section =="
if [ -f charts/plane-enterprise/templates/config-secrets/app-env.yaml ]; then
nl -ba charts/plane-enterprise/templates/config-secrets/app-env.yaml | sed -n '70,90p'
fi
echo
echo "== values references for celery_broker_pool_limit =="
rg -n "celery_broker_pool_limit|broker_pool_limit|CELERY_BROKER_POOL_LIMIT" -S .Repository: makeplane/helm-charts
Length of output: 610
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
# Read-only behavioral probe of Helm Go `default` equivalent semantics as described:
# numeric 0 is treated empty by the chart template, so an explicit 0 via default 10 becomes 10.
from functools import reduce
values = {'env': {'celery_broker_pool_limit': 0}}
actual = values.get('env') or {}
actual = actual.get('celery_broker_pool_limit')
result = actual if actual is not None and actual != '' else 10
print('env.celery_broker_pool_limit 0 ->', result)
values = {'env': {'celery_broker_pool_limit': None}}
actual = values.get('env') or {}
actual = actual.get('celery_broker_pool_limit')
result = actual if actual is not None and actual != '' else 10
print('missing default fallback key ->', result)
PYRepository: makeplane/helm-charts
Length of output: 230
🌐 Web query:
Helm chart function default zero numeric treated empty documentation
💡 Result:
In Helm charts, the default function treats a numeric 0 as an "empty" value [1][2]. Consequently, if you use {{ default "fallback".Values.myValue }} and.Values.myValue is set to 0, Helm will return "fallback" instead of 0 [3][4]. This behavior is intended by the template engine, which considers the following types "empty" [1][2]: - Numeric: 0 - String: "" - Lists: [] - Dictionaries: {} - Boolean: false - Nil (null) Because 0 is categorized as empty, the default function cannot natively distinguish between a value that is missing (nil) and a value that is explicitly set to 0 [4][5]. Workarounds If your use case requires treating 0 as a valid value while still providing a default for truly missing/null values, you must implement logic to explicitly check the value instead of relying on the default function [3][4]: 1. Explicit Check with if/else: You can check if the value is specifically 0 before applying a default [5]: {{- if eq (toString.Values.myValue) "0" }} myValue: 0 {{- else }} myValue: {{ default "fallback".Values.myValue }} {{- end }} 2. Kind Checking: Some developers use the kindIs function to verify if the value is set before deciding how to handle it, which helps avoid type-related comparison errors [6]: {{- if or (kindIs "int64".Values.myValue) (kindIs "float64".Values.myValue) }} myValue: {{.Values.myValue }} {{- else }} myValue: {{ default "fallback".Values.myValue }} {{- end }} 3. Setting Defaults in values.yaml: The most robust practice is to define static default values directly in your values.yaml file rather than using the default function in templates [7][8][5]. This ensures the value is explicitly defined and avoids the ambiguity of the default function [5].
Citations:
- 1: https://helm.sh/docs/chart_template_guide/function_list
- 2: https://github.com/helm/helm-www/blob/main/versioned_docs/version-3/chart_template_guide/function_list.md
- 3:
defaulttemplate function treats 0 value as unspecified helm/helm#13036 - 4: Wrong interpretation of Zero in templates files when having a default value helm/helm#3164
- 5: Wrong interpretation of Zero in templates files when having a default value helm/helm#3164
- 6: https://github.com/cert-manager/cert-manager/pull/6713/files/2b14b3234d3b853c7805b237066aef5c81927fa3
- 7: https://v3-1-0.helm.sh/docs/chart_template_guide/functions_and_pipelines/
- 8: https://helm.sh/docs/chart_template_guide/functions_and_pipelines/
🌐 Web query:
Celery broker_pool_limit 0 disable connection pool documentation
💡 Result:
According to official Celery documentation, setting the broker_pool_limit configuration option to 0 or None will disable the broker connection pool [1][2][3]. When disabled, Celery will establish and close a connection for every individual use [1][2][3]. While this is the documented behavior, it is worth noting that some users have reported issues in recent versions of Celery where setting this parameter to 0 or None does not appear to have the expected effect of disabling the pool, leading to potential connection exhaustion in certain client configurations [4]. The broker connection pool has been enabled by default since Celery version 2.5, with a default limit of 10 connections [1][2][3]. Increasing this limit is generally recommended to minimize contention in environments with high numbers of active threads or green-threads [5][1][3].
Citations:
- 1: https://docs.celeryq.dev/en/3.0/userguide/configuration.html
- 2: https://docs.celeryq.dev/en/v4.0.1/userguide/configuration.html
- 3: https://docs.celeryq.dev/en/v5.6.1/userguide/configuration.html
- 4: broker_pool_limit has no effect when set to 0 or None celery/celery#8416
- 5: https://docs.celeryq.dev/en/stable/userguide/optimizing.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("charts/plane-enterprise/templates/config-secrets/app-env.yaml")
text = p.read_text()
idx = text.find("CELERY_BROKER_POOL_LIMIT")
print(text[idx-120: idx+160])
path = Path("charts/plane-enterprise/values.yaml")
print("\n=== values.yaml celery_broker_pool_limit context ==")
lines = [ln.splitlines()[0] for ln in path.read_text().splitlines()]
for i, line in enumerate(lines, 1):
if "celery_broker_pool_limit" in line:
for offset in range(-3, 4):
j = i + offset
if 0 < j <= len(lines):
print(f"{j}: {lines[j-1]}")
PYRepository: makeplane/helm-charts
Length of output: 642
Preserve an explicit 0 broker pool limit.
default treats numeric 0 as empty, so --set env.celery_broker_pool_limit=0 renders CELERY_BROKER_POOL_LIMIT: "10". Since Celery allows 0 and None to disable the connection pool, use presence-based handling such as hasKey or validate/reject 0 explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/plane-enterprise/templates/config-secrets/app-env.yaml` at line 82,
Update the CELERY_BROKER_POOL_LIMIT template expression to preserve an
explicitly configured value of 0 instead of allowing default to replace it with
10. Use presence-based handling such as hasKey on
.Values.env.celery_broker_pool_limit, while retaining 10 only when the key is
absent.
…les for improved configuration
1 participant
What
Adds 5 new env vars to the
plane-enterprisechart across two ConfigMaps:app-varsConfigMap (templates/config-secrets/app-env.yaml):pi-api-varsConfigMap (templates/config-secrets/pi-api-env.yaml):Defaults declared in
values.yaml; Rancher form entries added inquestions.yml; README table updated.Chart version bumped
3.0.0 → 3.0.1.Why
Reported in #261 (leedsjb): a 50% silent CSV export failure rate caused by two compounding issues:
Gunicorn worker rotation —
--max-requests 1200is hardcoded in the image entrypoint, rotating each worker every ~1,200–2,200 requests. Each rotation produces a ~10–30 s AMQP reconnect window. During that window,task.delay()either hits a broken connection (returning without raising) or writes to a kernel-accepted-but-broker-closed socket — both paths silently discard the task, leaving theexportersrecord stuck in"queued"forever.No publish retry + unbounded broker pool — without
CELERY_TASK_PUBLISH_RETRY, Kombu treats a failed publish as final. Without a pool limit, stale connections accumulate and are reused without health-checking.PLANE_INTERNAL_API_HOSTis added so PI can reach the backend API directly over the cluster network without going through the ingress.Scope / behavior
GUNICORN_MAX_REQUESTS0(disabled)1200; this env allows override.GUNICORN_MAX_REQUESTS_JITTER0CELERY_TASK_PUBLISH_RETRYTrueCELERY_BROKER_POOL_LIMIT10PLANE_INTERNAL_API_HOSThttp://<release>-api.<ns>.svc.<domain>:8000.GUNICORN_MAX_REQUESTSandGUNICORN_MAX_REQUESTS_JITTERonly take effect if the image entrypoint reads these env vars (the fix in the application image is a prerequisite for the rotation changes to work).CELERY_TASK_PUBLISH_RETRYandCELERY_BROKER_POOL_LIMITare Celery settings consumed by the Django/Celery app directly.PLANE_INTERNAL_API_HOSTis rendered unconditionally wheneverservices.pi.enabled=true(the template is already gated by that condition).Testing
helm lint— clean, 0 failures.helm templateoutput (defaults):helm templatewith--set env.gunicorn_max_requests=1200 --set env.gunicorn_max_requests_jitter=200 --set env.celery_task_publish_retry=false --set env.celery_broker_pool_limit=5:Related
🤖 Generated with Claude Code
Summary by CodeRabbit