Skip to content

Fix review-app renderer resource ratio#787

Merged
justin808 merged 1 commit into
masterfrom
jg-codex/fix-review-app-resource-ratio
Jul 18, 2026
Merged

Fix review-app renderer resource ratio#787
justin808 merged 1 commit into
masterfrom
jg-codex/fix-review-app-resource-ratio

Conversation

@justin808

@justin808 justin808 commented Jul 18, 2026

Copy link
Copy Markdown
Member

Why

A fresh-default replay after #785 reached provider resource validation and exposed a second checked-in template constraint: the renderer used 512 MiB with 50 millicores, a ratio of 10.24, while the provider requires the memory-to-CPU ratio to be strictly below 8.

This follow-up raises only the renderer CPU to the conventional 100 millicores. Memory remains 512 MiB, producing a valid ratio of 5.12.

What changed

  • Change the renderer CPU from 50m to 100m.
  • Extend the rendered-template gate to enforce the provider ratio with a strict < 8 comparison.
  • Lock the existing memory, runtime environment, image, arguments, ports, 120-second liveness grace, readiness behavior, autoscaling, firewall, identity, and workload shape.

TDD and validation

  • RED on merged master: the provider readiness stage passed, then the new structural regression failed at ratio 10.24 and the old CPU value.
  • GREEN after the one-line resource fix: bin/conductor-exec bin/test-cpflow-github-flow passed at ratio 5.12, including YAML parsing and actionlint.
  • .agents/bin/validate passed: 58 files, no offenses.
  • .agents/bin/test passed the full repository setup/build/lint/test path.
  • bash -n, git diff --check, exact owned-path audit, and fresh origin/master comparison passed.

Replay history and issue state

Summary by CodeRabbit

  • Performance

    • Increased the node-renderer workload’s CPU allocation from 50m to 100m.
  • Bug Fixes

    • Expanded configuration validation to detect mismatches in runtime settings, environment variables, probes, networking, and resource ratios.
    • Added checks for LOG_LEVEL and NODE_OPTIONS configuration values.

@github-actions

Copy link
Copy Markdown

🚀 Quick Review App Commands

Welcome! Here are the commands you can use in this PR:
They require the repository to have cpflow review apps configured, including the CPLN_TOKEN_STAGING secret.

+review-app-deploy

Deploy your PR branch for testing.

+review-app-delete

Remove the review app when done.

+review-app-help

Show detailed instructions, environment setup, and configuration options.

Comment +review-app-help for full setup details.

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 342a24fd-9558-4e60-a46f-d81f8fcb39ba

📥 Commits

Reviewing files that changed from the base of the PR and between 8266366 and c98c64c.

📒 Files selected for processing (2)
  • .controlplane/templates/node-renderer.yml
  • bin/test-cpflow-github-flow

Walkthrough

The node-renderer CPU request increases from 50m to 100m. The associated Ruby validation now parses YAML into intermediate structures and checks workload fields, environment variables, defaults, firewall settings, probes, and provider resource ratios.

Changes

Node renderer validation

Layer / File(s) Summary
Renderer resource request
.controlplane/templates/node-renderer.yml
The node-renderer container CPU request changes from 50m to 100m.
Template drift checks
bin/test-cpflow-github-flow
YAML parsing and assertions are expanded to validate container settings, defaults, firewall configuration, probes, and a provider resource ratio below 8.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adjusting the review-app renderer resource ratio to satisfy the provider limit.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jg-codex/fix-review-app-resource-ratio

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review: Fix review-app renderer resource ratio

Overview

Small, well-scoped infra fix: bumps the node-renderer container CPU from 50m to 100m (memory unchanged at 512Mi) to satisfy the Control Plane provider's memory:CPU ratio constraint (< 8), and hardens bin/test-cpflow-github-flow with a much more thorough structural lock-down of node-renderer.yml plus an explicit ratio assertion so this class of regression fails fast locally instead of only at deploy time.

Code quality

  • The core fix (.controlplane/templates/node-renderer.yml) is a minimal, correct one-line change consistent with the PR's stated goal (ratio 10.24 → 5.12).
  • The added Ruby assertions in bin/test-cpflow-github-flow are defensive and readable: the CPU/memory regex match guards against non-m/Mi unit formats and avoids a divide-by-zero via cpu_match[:millicores].to_i.positive? before computing the ratio.
  • Good use of %w[...] key-set assertions (workload keys changed, spec keys changed, container keys changed) to catch unintended schema drift, matching the PR's explicit "lock the existing shape" goal.

Minor observations (non-blocking)

  • The exact-keys assertions (e.g. container.keys.sort == %w[...]) will need a manual update any time a legitimate new field is added to the container spec (e.g. a future startupProbe alternative or extra env var). That's presumably intentional as a tripwire, but worth knowing it adds friction to otherwise-routine template edits.
  • The < 8 ratio threshold is asserted directly against raw MiB / millicores, without unit conversion to GiB/vCPU. If Control Plane's actual provider constraint is expressed in GiB/vCPU rather than raw MiB/millicores, the effective threshold in this script's units would be ~8.19 rather than 8, making this check slightly more conservative than the real provider limit. Given the current ratio (5.12) has comfortable margin either way, this isn't a functional problem today — just worth a one-line comment noting where the 8 constant comes from (provider docs/support ticket) so future maintainers don't need to reverse-engineer it if they ever need to tune resources closer to the limit.

Security / performance

  • No security implications; this only touches CI-local validation tooling and a resource request value.
  • Doubling CPU request (50m → 100m) for a single-worker capacityAI-managed workload is a negligible cost/performance change and consistent with the "minimum starting point, let capacityAI scale" convention already used for the rails workload template.

Test coverage

  • Good: the PR follows a clear RED/GREEN TDD workflow, and the new script assertions materially increase coverage of the template's shape, not just the probe/ratio fix itself.

Overall: low-risk, well-tested, and does what it says. No blocking issues found.

@justin808

Copy link
Copy Markdown
Member Author

Address-review follow-up on the current-head review:

  • The exact-key checks are intentional tripwires for this deployment template; legitimate schema changes should update the preserved-invariant contract deliberately.
  • The disposable replay established the validator’s exact Memory(MiB) / CPU(millicore) < 8 rule, so the raw-unit comparison and literal threshold match the enforced contract. At the current values, the ratio is 5.12.
  • I am declining the optional comment-only change because the assertion and failure message already name the provider ratio, and a nit-only push would restart the green final-candidate review cycle.

No code change or push is needed.

@justin808

Copy link
Copy Markdown
Member Author

Address-review summary

Scan scope: full current-head history via the explicit check all reviews override.

Mattered

  • None.

Optional

  • Declined with no code change: the threshold-origin comment suggestion. The disposable replay established the exact raw-unit < 8 validator contract, the current assertion and failure message identify the provider ratio, and a comment-only push would restart the green final-candidate review cycle.
  • Evidence-backed rationale recorded. The exact-key assertions remain intentional tripwires.

Skipped

  • Two automated help/review-status posts contained no actionable code or documentation request; no action.

Current-head readiness

  • Exact head: c98c64c2822194c259dc859af0397d56e5bca222.
  • READY: 7 configured checks are terminal with 0 pending and 0 failing; 0 review threads are unresolved.
  • No code change or push was needed.

Next default scan starts after this comment. Say check all reviews to rescan the full PR.

@justin808

Copy link
Copy Markdown
Member Author

Final-candidate gate update for c98c64c2822194c259dc859af0397d56e5bca222:

  • Exact-head QA: clean.
  • Fresh maker-distinct Sol/xhigh audit: clean; no blocking or discuss findings.
  • Current-head CI and review inventory: green, zero unresolved threads.
  • The simplification run exceeded its bounded window and was stopped with zero workspace edits; the audited exact head remains unchanged.
  • The maintainer-provided batch authority is auto_merge_when_gates_pass; this is the explicit approval to promote this exact follow-up head from draft once its ready-state reviewer wave is clear.

This follow-up preserves every probe/runtime/security invariant from merged PR #785 and changes only the renderer CPU ratio plus replayable regression coverage. Fresh-default deployment replay remains tracked by #784 after merge.

@justin808
justin808 marked this pull request as ready for review July 18, 2026 08:15
@greptile-apps

greptile-apps Bot commented Jul 18, 2026

Copy link
Copy Markdown

Greptile Summary

This PR corrects the renderer resource ratio and strengthens its template checks. The main changes are:

  • Raises renderer CPU from 50m to 100m.
  • Enforces a strict memory-to-CPU ratio below 8.
  • Locks the expected workload structure and runtime settings.

Confidence Score: 5/5

This looks safe to merge.

  • The new allocation has a valid ratio of 5.12.
  • The ratio check correctly rejects the previous 10.24 value.
  • No blocking issues were found in the changed code.

Important Files Changed

Filename Overview
.controlplane/templates/node-renderer.yml Raises the renderer CPU allocation while preserving its memory and runtime configuration.
bin/test-cpflow-github-flow Adds exact template assertions and validates the provider resource ratio for the checked-in renderer values.

Reviews (1): Last reviewed commit: "Keep renderer resources within provider ..." | Re-trigger Greptile

Comment thread bin/test-cpflow-github-flow
Comment thread bin/test-cpflow-github-flow
@claude

claude Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review

Overview

Small, well-scoped follow-up to #785/#786: bumps the node-renderer container's CPU from 50m to 100m (memory unchanged at 512Mi), bringing the memory:CPU ratio from 10.24 down to 5.12, under the control-plane provider's required < 8 ratio. The companion change extends bin/test-cpflow-github-flow's inline Ruby check to assert this ratio (and a broad set of other template fields) so the regression can't silently reappear.

Correctness

  • Ratio math is correct and matches the "strictly below 8" requirement (resource_ratio < 8), and 512/100 = 5.12 checks out.
  • The regex-based CPU/memory parsing (\A(?<millicores>\d+)m\z / \A(?<mebibytes>\d+)Mi\z) is guarded against non-numeric or zero CPU (division-by-zero avoided via .positive?), and fails closed with a clear message if units don't match the expected m/Mi format.
  • The new expected_* fixtures (env, default options, firewall config, etc.) all match the current node-renderer.yml content — verified by reading both files side by side.

Suggestions (left as inline comments)

  1. The 8 ratio threshold is a magic number whose meaning (provider-imposed max memory:CPU ratio) only lives in the PR description — worth a named constant + comment in the script itself for future readers.
  2. The new keys.sort == structural assertions effectively snapshot the entire template shape (workload/spec/container keys). This is presumably intentional given the repeated regressions (Preserve review-app renderer startup grace #785, QA: Verify repaired review-app gate (DO NOT MERGE) #786), but it means any future legitimate field addition to the template will break this script with a generic "X keys changed" message. Consider including the actual vs. expected key sets in the failure message to make future triage faster.

Other notes

  • No security concerns — this is CPU/memory sizing for an internal control-plane workload template, no secrets or external inputs involved.
  • Test coverage is a clear net positive: the script now pins down probe grace, env vars, ports, firewall config, autoscaling, and identity link in addition to the new ratio check, closing off a whole class of "silent template drift" regressions.
  • Nothing else in the diff (the CPU value change itself) raises concerns; it's a minimal, targeted fix.

Overall: solid, low-risk fix with good regression coverage. The two suggestions above are minor maintainability nits, not blockers.

@justin808

Copy link
Copy Markdown
Member Author

Address-review summary

Scan scope: normal cutoff mode, reviewing ready-state activity after the previous summary at 2026-07-18T07:56:51Z.

Mattered

  • None.

Optional

Skipped

  • The final-candidate gate update was coordination/status evidence with no code request; no action.
  • Greptile reported the exact head safe with no blocking findings; no action.
  • Claude’s general summary duplicated the two inline maintainability nits and reported no blocker; handled through the resolved threads above.

Current-head readiness

  • Exact head: c98c64c2822194c259dc859af0397d56e5bca222.
  • READY: 10 configured checks are terminal with 0 pending and 0 failing; both ready-state reviewer checks passed; 0 review threads are unresolved.
  • No code change or push was needed.

Next default scan starts after this comment. Say check all reviews to rescan the full PR.

@justin808
justin808 merged commit 803b592 into master Jul 18, 2026
27 checks passed
@justin808
justin808 deleted the jg-codex/fix-review-app-resource-ratio branch July 18, 2026 08:22
@github-actions

github-actions Bot commented Jul 18, 2026

Copy link
Copy Markdown

✅ Review App Deleted

Review app for PR #787 is deleted

🎮 Control Plane Console
📋 View Workflow Logs

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.

1 participant