Skip to content

fix(helm): APP-16424 lengthen startup budget and test probe fallbacks - #42574

Merged
wyattwalter merged 1 commit into
fix/app-16424-helm-probesfrom
WW-422-helm-probe-tweaks
Nov 6, 2026
Merged

wyattwalter merged 1 commit into
fix/app-16424-helm-probesfrom
WW-422-helm-probe-tweaks

Conversation

@wyattwalter

Copy link
Copy Markdown
Contributor

Problem: The 300 s default startup window in #42571 can be too short for a first boot against a remote MongoDB. Deploy previews have no startup override. Also, the template fallbacks and the values.yaml defaults can drift apart without any test failing.
The fix: Raise the default startup window to 600 s. Give deploy previews a 30 min startup window. Add a test that fails when the template fallbacks and values.yaml disagree.

This PR targets the #42571 branch, fix/app-16424-helm-probes. Merge it into that branch when convenient. It does not change the liveness or readiness probes.

Changes

  • values.yaml and templates/deployment.yaml: the startup probe failureThreshold default changes from 30 to 60. With the 10 s period, the window is 600 s. On a fresh install, the server runs every migration before it listens. The incident in APP-16275 took more than 3.5 min to reach the newer migrations on a remote Atlas cluster. The startup probe stops after its first success, so the larger value has no cost on a running server.
  • tests/probes_test.yaml: the new case "template fallbacks match the values.yaml defaults when probes is unset" sets probes: null. It asserts the same probe blocks as the defaults test, through shared YAML anchors. helm upgrade --reuse-values from 3.9.1 renders through the dig fallbacks, because 3.9.1 has no probes in values.yaml. If someone changes one set of defaults and not the other, this test fails.
  • scripts/deploy_preview.sh: sets probes.startupProbe.periodSeconds=10 and failureThreshold=180, for 30 min. The period is explicit, so the window stays the same on published charts that still default to a 60 s period.
  • README.md and the snapshot are regenerated. values.schema.json is unchanged.

After merge, update the probe table in the #42571 description: startup is 10 s / 5 s / 60, for 600 s to boot.

Verification

From deploy/helm:

  • helm unittest .: 21 suites, 100 tests, and 11 snapshots pass.
  • Mutation checks on the new test:
    • The liveness fallback in the template was changed from 18 to 17. The test failed.
    • values.yaml and the shared expected value were both changed from 60 to 90, with the template unchanged. The test failed.
  • helm schema and helm-docs --sort-values-order file were regenerated. Only README.md changed.
  • helm lint . passes. The appVersion unmarshal message also appears on the base branch.
  • bash -n scripts/deploy_preview.sh passes.
  • A separate local k3s run of appsmithorg/appsmith#42571 at 3453d6bc tested upgrades from 3.9.1 with --reuse-values, with and without customized probes, on Helm 3.19.2 and 4.3.0. The upgrades succeeded with the expected probe values. This PR changes only one default number in the template, so those results still apply.

Rollback

Revert this commit on fix/app-16424-helm-probes. Nothing in it is stateful.

Linear: APP-16424

🤖 Generated with Claude Code

- Raise the default startup probe failureThreshold from 30 to 60
  (600 s at a 10 s period). A first boot against a remote MongoDB
  runs every migration and can take longer than 300 s.
- Add a helm-unittest case that unsets `probes` and asserts the
  template fallbacks render the same probes as the values.yaml
  defaults. `helm upgrade --reuse-values` from 3.9.1 renders through
  those fallbacks, so a mismatch now fails CI.
- Set the deploy preview startup probe to 10 s x 180 (30 min). The
  period is explicit so the budget is the same on charts with a 60 s
  default period.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Summary

Summary by CodeRabbit

  • Bug Fixes
    • Increased the default startup allowance from 300 to 600 seconds, giving slower-starting deployments more time to become ready.
  • Documentation
    • Clarified the preview deployment’s startup probe settings and noted that its first boot runs migrations against a remote database cluster.

The Helm startup probe default failure threshold increases from 30 to 60, which allows 600 seconds at the configured 10-second period. A test checks that probes: null renders the default probes. The preview deployment sets the startup probe threshold to 180 and documents its first-boot migrations against a remote Atlas cluster.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • appsmithorg/appsmith#42571: Sets the startup probe default to 30 and adds probe tests; this PR increases the default to 60 and extends the fallback tests.

Suggested labels: CI

Suggested reviewers: subrata71

Merge Risk: Minimal · up to 788e0

The longer startup windows introduce no identified merge-blocking risk.

Pre-merge checks: Passed 5

Check name Status Explanation
Title check Passed The title describes the longer startup budget and probe fallback test.
Description check Passed The description covers the change, motivation, issue, and verification. It omits the Communication choice and does not use the template headings or Fixes syntax.
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files. (4 skipped: 4 unsupported.)

Finishing Touches

Generate docstrings

  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Automatic approval policy

Commit: 788e08aa9591.

Setting Effective value Source
Automatic approvals Enabled Global configuration
Merge risk Low Global configuration
Security risk Not set Global configuration

Not set means no limit was configured after inheritance. Use @coderabbitai configuration to inspect the resolved settings and their sources.

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 3453d6b and 788e08a.

Files ignored due to path filters: 1

  • deploy/helm/tests/__snapshot__/defaults_snapshot_test.yaml.snap is excluded by !**/*.snap
Files selected for processing: 5

  • deploy/helm/README.md
  • deploy/helm/templates/deployment.yaml
  • deploy/helm/tests/probes_test.yaml
  • deploy/helm/values.yaml
  • scripts/deploy_preview.sh

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Recent review details

Additional context used

Code guidelines: 2

deploy/helm/AGENTS.md — auto-discovered
.cursor/rules/regen-helm-schema.mdc — auto-discovered
Path-based instructions: 2

Source excerpt: When adding a new value, include at least the `# --` and `# `@section` --` lines so it lands in the correct README section.

CodeRabbit inference engine (deploy/helm/AGENTS.md)

Files:

  • deploy/helm/values.yaml
Source excerpt: Descriptions cannot contain `;` — it's the annotation separator inside `# `@schema``.

CodeRabbit inference engine (.cursor/rules/regen-helm-schema.mdc)

Files:

  • deploy/helm/values.yaml

@linear-code

linear-code Bot commented Nov 6, 2026

Copy link
Copy Markdown

APP-16424

@wyattwalter
wyattwalter marked this pull request as ready for review November 6, 2026 18:34
@wyattwalter
wyattwalter requested a review from a team as a code owner November 6, 2026 18:34
@wyattwalter
wyattwalter added this pull request to stack #42575 November 6, 2026 18:37

@subrata71 subrata71 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@wyattwalter
wyattwalter enabled stacked auto-merge (squash) November 6, 2026 18:51
@wyattwalter
wyattwalter disabled stacked auto-merge November 6, 2026 18:51
@wyattwalter
wyattwalter enabled stacked auto-merge (squash) November 6, 2026 18:52
@subrata71
subrata71 disabled stacked auto-merge November 6, 2026 18:55
@wyattwalter
wyattwalter merged commit 788e08a into release Nov 6, 2026
30 checks passed
@wyattwalter
wyattwalter deleted the WW-422-helm-probe-tweaks branch November 6, 2026 18:55
An error occurred while trying to automatically change base from fix/app-16424-helm-probes to release November 6, 2026 19:43
wyattwalter added a commit that referenced this pull request Nov 6, 2026
## Description

Saving some admin settings restarts the server inside the Appsmith
container for about 45 seconds. The chart's probes ran every 60 s with
Kubernetes' default 1 s timeout and a failure threshold of 3. When three
restarts happened back to back, each one could land on a single liveness
check. Kubernetes counted three failures in a row and killed the
container mid-boot. This happened on nightly-saml on 2026-11-06 during
the SSO Sanity run.

`/api/v1/health` checks Mongo (1 s timeout) and Redis (3 s timeout) in
parallel. A slow but working server could therefore fail a 1 s probe.

Fixes https://linear.app/appsmith/issue/APP-16424

### Changes

- `templates/deployment.yaml`: each probe now sets `timeoutSeconds`,
which can be overridden. New defaults:

  | Probe | periodSeconds | timeoutSeconds | failureThreshold | Effect |
  |---|---|---|---|---|
  | startup | 60 → 10 | 1 → 5 | 3 → 60 | 600 s to boot |
| liveness | 60 → 10 | 1 → 5 | 3 → 18 | restart after 180 s of
continuous failure |
| readiness | 60 → 10 | 1 → 5 | 3 | unready after 30 s, ready again
within 10 s |

One successful check resets the liveness count. A 45 s restart causes at
most about 5 failures, so even several restarts in a row cannot reach
18.
- `values.yaml`: documents the `probes` value, which the template
already read through `dig` but no one had documented. Keys: `port`,
`api` (the HTTP path), `periodSeconds`, `timeoutSeconds`,
`failureThreshold`. The `dig` fallbacks in the template match the new
defaults.
- `values.schema.json` and `README.md`: regenerated.
- Startup gets 600 s because a first boot runs every migration before
the server listens. On a remote Atlas cluster this took more than 3.5
minutes (APP-16275). The startup probe stops after its first success, so
the longer window costs nothing on a running server. (From #42574 by
@wyattwalter, merged into this branch.)
- `values.schema.json`: probe `periodSeconds`, `timeoutSeconds` and
`failureThreshold` accept an integer of at least 1, or a string of
digits with no leading zero. Chart 3.9.1 rendered quoted numbers such as
`"60"` as integers, so an upgrade that keeps such an override still
passes.
- `tests/probes_test.yaml`: new helm-unittest suite. It covers the
defaults, a partial override, and schema rejection of bad values. It
also checks that the template fallbacks equal the `values.yaml` defaults
when `probes` is unset. That matters because `helm upgrade
--reuse-values` from 3.9.1 renders through the fallbacks. The tests fail
against the old template and schema, and pass with this change.
- `scripts/deploy_preview.sh`: deploy previews set the startup probe to
10 s × 180 (30 minutes). The period is set explicitly, so the window
holds on published charts that still default to 60 s. Published charts
do not render `timeoutSeconds` yet, so previews keep the 1 s timeout
until a chart with this change is published. (From #42574.)
- `Chart.yaml`: 3.9.1 → 3.9.2. The snapshot is updated to match.

Call sites checked: the probes are defined only in
`templates/deployment.yaml`, which renders both the StatefulSet and the
Deployment. No other template defines a probe.

### Impact on existing instances

| State | What happens |
|---|---|
| Fresh install | Gets the new defaults. Liveness and readiness each
call `/api/v1/health` every 10 s (was every 60 s). That is 12 calls a
minute instead of 2. Each call is a Mongo ping and a Redis ping. |
| Upgrade from default | The pod spec changes, so `helm upgrade` rolls
the pod once. After that, behaviour matches a fresh install. |
| Upgrade from customized `probes` | Keys the user set still win,
because Helm merges user values over chart defaults key by key. Keys the
user did not set now take the new defaults. For example, a user who set
only `livenessProbe.failureThreshold: 3` now gets a 10 s period and a 5
s timeout, so the restart window shrinks from about 180 s to 30 s. Users
who tuned `failureThreshold` for a 60 s period should also set
`periodSeconds: 60`. The schema now validates `probes`. Quoted numbers
such as `failureThreshold: "5"` still work. Values that are not positive
integers, such as `0` or `"ten"`, now fail `helm install`/`upgrade` with
a schema error. Before, they reached Kubernetes and failed there. |
| Rollback to 3.9.1 | Restores the old probe values (60 s period, 1 s
timeout, threshold 3). An override that sets `probes.*.timeoutSeconds`
has no effect on 3.9.1, because that template ignores the key. 3.9.1 has
no schema for `probes`, so the rollback does not fail. |

### CE → EE sync

The EE chart is at 3.9.3, so the hourly sync will conflict on the
`Chart.yaml` version and the snapshot file. Resolution: set the EE
version to 3.9.4 and regenerate the snapshot with `helm unittest -u`. I
made a local copy of that end state on the EE side and checked it: 99/99
unit tests pass, the schema and README regenerate cleanly, and `helm
lint` passes.

### Follow-up (not in this PR)

The probe port defaults to `80` even when `HTTPContainerPort` is set. A
user who sets `HTTPContainerPort` must also set every `probes.*.port`.

## Automation

Not run. Cypress runs against the Docker image, not a Helm install, so
it cannot exercise this change. The chart's own CI covers it:
helm-unittest, the values schema check and helm-docs.

## Testing

Run locally from `deploy/helm`:

- `helm lint .` passes.
- `helm unittest .`: 21 suites, 103 tests pass.
- `helm schema …` and `helm-docs --sort-values-order file --dry-run`
both match the committed files.
- `helm lint . --set probes.livenessProbe.timeoutSeconds=0` is rejected
by the schema.
- Chart 3.9.2 is not yet published at helm.appsmith.com.

- [ ] Client unit tests
- [ ] Server unit tests
- [ ] Cypress
- [ ] Playwright
- [ ] Deploy preview
- [x] Not applicable

## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No

🤖 Generated with [Claude Code](https://claude.com/claude-code)


<!-- This is an auto-generated comment: Cypress test results  -->
> [!WARNING]
> Tests have not run on the HEAD
788e08a yet
> <hr>Fri, 06 Nov 2026 18:56:14 UTC
<!-- end of auto-generated comment: Cypress test results  -->

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Wyatt Walter <wyattwalter@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants