Skip to content

fix: use effective memory limit for node ram - #4417

Open
Raubzeug wants to merge 2 commits into
mainfrom
fix/node-memory-hard-limit
Open

Raubzeug wants to merge 2 commits into
mainfrom
fix/node-memory-hard-limit

Conversation

@Raubzeug

@Raubzeug Raubzeug commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

When a node has a configured YDB memory limit below its host or cgroup limit, RAM currently uses the larger limit while Detailed Memory uses the effective YDB limit. For example, a 24 GiB YDB limit on a 32 GiB host produces inconsistent capacities and progress bars.

Use one memory calculation for both columns and the node details page. Keep valid MemoryUsed authoritative so the displayed consumption matches backend pagination and sorting, and use detailed consumption only when that value is unavailable. Prefer MemoryStats.HardLimit with a fallback to the legacy MemoryLimit; use the same effective limit in the detailed popup. Missing and invalid values remain unknown rather than producing a zero limit or a misleading progress bar. Keep the usage breakdown available when no limit is known.

When the backend reports allocated memory without anonymous RSS, allocator caches are shown as a separate informational metric rather than included in the usage bar. This keeps RAM and Detailed Memory consistent with the backend consumption value.

Request MemoryDetailed when RAM is selected and in Versions. This expands the existing SystemState response with cached memory statistics; it does not add a new metrics collection request. Versions fetches all nodes, so the additional response payload scales with cluster size. The node details request already includes all fields.

Closes #3267.

Validation:

  • npm run typecheck
  • npm run lint
  • npm test -- --runInBand: 2,120 tests passed across 235 suites
  • npm run build:embedded and npm run package
  • 40 Playwright checks in Chromium and Safari against the production build with mocked viewer APIs, covering effective limits, popup fallbacks and unknown limits, server-side RAM sorting, allocator cache accounting, legacy and partial responses, and node details.

Refresh the Chromium and Safari Linux error-card snapshots from visually reviewed CI captures: the additional request field wraps the displayed URL onto one more line. Linux screenshot comparison will be rechecked by CI.

RetriggerConfidence Score: 4/5

The loss of the Detailed Memory breakdown is non-blocking and does not make this PR unsafe to merge.

Fix All in CodexFindings

  1. P2 Memory breakdown disappears ▶
Fix with agent prompt
### Issue 1
src/components/MemoryViewer/MemoryViewer.tsx:71-73
When a node reports memory usage and segment values but no hard or legacy limit, this return displays only the usage value. The Detailed Memory cell loses its hover breakdown, so operators cannot inspect the available cache, query, and memtable figures. Keep the breakdown available while hiding limit-dependent progress. This loss of detail is non-blocking.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR centralizes node memory usage and limit selection across RAM, Detailed Memory, and node details, and requests detailed statistics for RAM and Versions. An operator viewing a node with usage and segment data but no memory limit needs the Detailed Memory breakdown; this change removes that breakdown.

Reviews (1) · Last reviewed commit: "fix: use effective YDB memory limit for ..."

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
1270 1267 0 3 0
Test Changes Summary ✨11

✨ New Tests (11)

  1. RAM agrees with Detailed Memory for effective hard limit (nodes/memoryLimit.test.ts)
  2. RAM agrees with Detailed Memory for allocator usage including caches (nodes/memoryLimit.test.ts)
  3. RAM agrees with Detailed Memory for legacy backend (nodes/memoryLimit.test.ts)
  4. RAM agrees with Detailed Memory for partial detailed stats (nodes/memoryLimit.test.ts)
  5. RAM agrees with Detailed Memory for missing hard limit (nodes/memoryLimit.test.ts)
  6. RAM agrees with Detailed Memory for zero hard limit (nodes/memoryLimit.test.ts)
  7. RAM agrees with Detailed Memory for invalid hard limit (nodes/memoryLimit.test.ts)
  8. RAM agrees with Detailed Memory for allocator caches outside backend usage (nodes/memoryLimit.test.ts)
  9. sorts RAM by the same backend usage that the cells display (nodes/memoryLimit.test.ts)
  10. keeps the detailed breakdown when both memory limits are missing (nodes/memoryLimit.test.ts)
  11. uses the effective YDB limit on the node page (nodes/nodeRam.test.ts)

Bundle Size: 🔺

Current: 65.13 MB | Main: 65.12 MB
Diff: +3.36 KB (0.01%)

⚠️ Bundle size increased. Please review.

ℹ️ CI Information
  • Test recordings for failed tests are available in the full report.
  • Bundle size is measured for the entire 'dist' directory.
  • 📊 indicates links to detailed reports.
  • 🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.

Copilot AI lite review requested due to automatic review settings September 25, 2026 13:35
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-25T14:03:22.396744Z 92bf702 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 65dcb3d364

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/components/nodesColumns/constants.ts

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The detailed memory popup must use the effective legacy limit when HardLimit is unusable.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

This PR unifies node memory usage and effective-limit calculations across RAM, detailed memory, Versions, and node details.

Changes:

  • Adds shared memory fallback and normalization logic.
  • Requests detailed memory data where needed.
  • Adds unit, integration, and Playwright coverage.
File Summary
tests/​suites/​nodes/​nodeRam.test.ts Adds node RAM regression coverage.
tests/​suites/​nodes/​memoryLimit.test.ts Tests effective limits and fallbacks.
src/​utils/​memory.ts Centralizes memory calculations.
src/​utils/​__test__/​memory.test.ts Tests memory normalization.
src/​containers/​Versions/​Versions.tsx Requests detailed memory data.
src/​components/​nodesColumns/​constants.ts Adds detailed memory requirements.
src/​components/​nodesColumns/​columns.tsx Applies normalized memory values.
src/​components/​nodesColumns/​__test__/​constants.test.ts Tests required fields.
src/​components/​MemoryViewer/​utils.ts Removes duplicated allocation logic.
src/​components/​MemoryViewer/​MemoryViewer.tsx Uses effective memory values; detailed segments still read raw HardLimit during legacy fallback.
src/​components/​FullNodeViewer/​FullNodeViewer.tsx Uses shared memory calculations.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/components/MemoryViewer/MemoryViewer.tsx
Comment thread src/components/MemoryViewer/MemoryViewer.tsx Outdated
@greptile-apps

greptile-apps Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Comments Outside Diff

These findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.

  • P2 Detailed Memory loses its breakdown when no memory limit is supplied ▶

    • Bug
      • Valid usage and segment values remain available, but the Detailed Memory cell becomes usage-only text and hovering it shows no breakdown.
    • Cause
      • At src/components/MemoryViewer/MemoryViewer.tsx:71-73, the undefined-capacity return exits before getMemorySegments and HoverPopup render.
    • Fix
      • Keep the segment breakdown and hover popup available when capacity is absent; omit only limit-dependent progress rendering.

This branch has not been deployed

No deployments
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.

Wrong memory limit in UI

2 participants