Skip to content

feat(deployment): make backend jvm options configurable - #7239

Draft
clonder wants to merge 1 commit into
mainfrom
feat/backend-heap-percentage
Draft

feat(deployment): make backend jvm options configurable#7239
clonder wants to merge 1 commit into
mainfrom
feat/backend-heap-percentage

Conversation

@clonder

@clonder clonder commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

follow up to #7077 (comment). the backend heap share becomes a backendMaxRamPercentage value (default 75, same as main, the other jvm flags stay fixed in the template), and values_preview_server.yaml sets 25 so previews go back to the jvm default heap. checked with helm lint --strict and helm template for default, e2e and preview values, the only rendered difference to main is MaxRAMPercentage=75.0 becoming =75. the new top level key changes valuesHash on every deployment, so the first upgrade after this restarts all pods once. lapis keeps its own hardcoded JVM_OPTS, out of scope here.

PR Checklist

  • All necessary documentation has been adapted.
    • schema description, shows up on the helm chart config docs page
  • The implemented feature is covered by appropriate, automated tests.
    • deployment config
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)
    • helm lint and template with default, e2e and preview values, schema rejects 0, 150 and non numbers

🚀 Preview: Add preview label to enable

@claude claude Bot added the deployment Code changes targetting the deployment infrastructure label Sep 3, 2026
@clonder
clonder marked this pull request as ready for review September 3, 2026 14:14
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 3m 27s —— View job


I'll analyze this and get back to you.

@clonder
clonder marked this pull request as draft September 3, 2026 14:19
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 3m 10s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @clonder's task in 2m 39s —— View job


Review: feat(deployment): make backend jvm options configurable

  • Read PR diff against origin/main
  • Check template rendering path and schema wiring
  • Check docs generation for the new key
  • Post findings

The mechanics are correct. backendMaxRamPercentage is defined in values.yaml, validated by the schema (so a missing/null/non-numeric value fails at install rather than producing a broken -XX:MaxRAMPercentage=), and "groups": ["services"] matches an existing group rendered by docs/src/content/docs/reference/helm-chart-config.mdx:41, with docs/package.json copying the schema at build time — so the docs page picks it up with no extra work. Top-level placement is consistent with the existing backendExtraArgs. Default 75 preserves current behaviour everywhere except previews.

Two comments left inline:

  1. The preview 25 is the only behavioural change, and it's the one thing not tested. It cuts the preview heap ceiling from ~2.25Gi to ~768Mi, while the MaxHeapFreeRatio=5/MinHeapFreeRatio=2 flags already in JVM_OPTS mean the JVM was returning idle heap to the OS anyway — so the memory saving on the shared cluster may be small while the OOM risk in the environment used for pre-merge testing is real. Lowering resources.backend.limits.memory in the same file caps the container footprint more directly.
  2. maximum: 100 in the schema permits a configuration that reliably gets the pod OOMKilled; suggested capping at 90.

Non-blocking: the PR title says "jvm options" but exactly one option is configurable. If more are expected later, a backendJvm: map would avoid growing the top-level namespace one key at a time — not worth changing for a single value. The valuesHash one-time restart and LAPIS keeping its hardcoded JVM_OPTS are both correctly called out in the description.

I could not run helm template/helm lint myself — those commands need approval in this environment, so my review of the rendering is static. CI runs both (.github/workflows/helm-schema-lint.yaml) against default, e2e and preview values.
· branch feat/backend-heap-percentage

Comment on lines +3 to +4
# Smaller backend heap, previews share one cluster
backendMaxRamPercentage: 25

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Preview resources.backend.limits.memory is 3Gi, so this drops the backend heap ceiling from ~2.25Gi to ~768Mi. Two things worth checking before merging:

  • MaxRAMPercentage is a ceiling, not a reservation. Combined with the already-set MaxHeapFreeRatio=5 -XX:MinHeapFreeRatio=2, the JVM already returns unused heap to the OS, so the steady-state RSS saving on the shared cluster may be near zero — the flag only bites once the heap actually needs to grow. What it does change is where the backend throws OutOfMemoryError.
  • Previews are the main pre-merge test environment. With 25% here and 75% in prod, a preview can OOM on a workload (large submission, big seqSet export) that prod handles fine, and the failure will look like an app bug. If the goal is to bound what a preview can consume on the shared cluster, lowering resources.backend.limits.memory in this same file is the knob that actually caps the container footprint — and the heap then scales with it automatically at 75%.

Was the 25% figure validated against an actual preview run (e2e or a manual submission), or only via helm template? The PR checklist says manual testing was helm lint/template only, which doesn't cover the runtime effect that is the entire point of the change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If 25% is the default, I guess no validation is needed, since setting 25% is actually as if we wouldn't change anything, right? That's what I got from the PR description.

Comment thread kubernetes/loculus/values.schema.json

@fhennig fhennig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

I guess there could be more written about why we picked 75% as the default, but I'm also ok with just picking it now and running with it - as you said it's already configured like this on main. It's hard to pick good defaults.

Another choice would be 25%, the JVM default, if we really wanted to play it safe.

But I'm fine with 75%

I'm approving but maybe wait a few days to give Cornelius some time to have a look as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment Code changes targetting the deployment infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants