fix: parse milli-quantities in parseUnitsOfBytes - #6817
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Pratikdeb07 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
|
Welcome @Pratikdeb07! |
803320f to
974aefb
Compare
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
Could you take a look at the commit messages in this PR? We follow a Linux kernel style for git commits — see the contributing guide and git log for examples.
Commits that need attention
fix: parse milli-quantities in parseUnitsOfBytes— Missingarea: descriptionprefix — e.g.frontend: HomeButton: Fix so it navigates to homeorbackend: config: Add enable-dynamic-clusters flag.
Commit guidelines
- Use atomic commits focused on a single change.
- Use the title format
<area>: <Description of changes>— description must start with a capital letter. - Keep the title under 72 characters (soft requirement).
- Explain the intention and why the change is needed.
- Make commit titles meaningful and describe what changed.
- Do not add code that a later commit rewrites; squash or reorder commits instead.
- Do not include
Fixes #NNin commit messages.
Good examples:
frontend: HomeButton: Fix so it navigates to homebackend: config: Add enable-dynamic-clusters flag
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect memory usage/percentage calculations when the Kubernetes Metrics API returns milli-quantities for memory (e.g. values ending in m, as commonly emitted by prometheus-adapter). It updates the shared frontend unit parser so memory values remain consistent across refreshes even when the provider alternates between Ki and m formats.
Changes:
- Add support for parsing byte quantities with the Kubernetes
m(milli) suffix inparseUnitsOfBytes()by dividing by 1000. - Add unit tests validating milli-byte parsing behavior for typical and fractional inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| frontend/src/lib/units.ts | Adds explicit handling for ...m byte quantities to prevent interpreting milli-values as raw bytes. |
| frontend/src/lib/units.test.ts | Adds coverage ensuring milli-byte quantities are parsed correctly and guard against regressions. |
974aefb to
c7e5770
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe unit parser recognizes Kubernetes milli-quantity values and divides them by 1000 before existing byte-unit parsing. Tests cover integer, large, and fractional milli-quantity inputs. ChangesMilli-quantity parsing
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Thanks for your review @illume ! I have updated the commit message. |
Signed-off-by: Pratik Debnath <pratikdebnath978@gmail.com>
c7e5770 to
deec6d4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
unlikelyzero
left a comment
There was a problem hiding this comment.
This only handles the 'm' suffix, but Kubernetes decimalSI also has 'u' (micro) and 'n' (nano) on the same family - same bug class, just less common. parseCpu right below in this same file already handles the full n/u/m ladder, and util.ts's normalizeUnit already handles all three for memory too, so the pattern to copy is already sitting in the codebase.
That duplication isn't theoretical either - resourceQuota/Details.tsx renders the same table row using both parsers: the Used/Hard columns go through normalizeUnit, and the Usage bar in that same row goes through parseRam/parseUnitsOfBytes. Before this PR they disagreed 1000x on milli values in that row; after it they agree on 'm' but still disagree on 'u'/'n' and on lowercase 'k' (which is the canonical k8s suffix - the regex here only matches uppercase 'K', so '1k' returns 1 instead of 1000, same 1000x bug in the same function).
A few smaller things inline.
| // Milli-quantity ex. "2687146666666m". Valid Kubernetes quantity syntax | ||
| // (e.g. emitted by prometheus-adapter for memory usage), but no unit is | ||
| // matched by the regex below, which would misread it as raw bytes. | ||
| if (value.endsWith('m')) { |
There was a problem hiding this comment.
Would extend this to cover 'u' and 'n' too, mirroring parseCpu a few lines down: /1000 for m, /1e6 for u, /1e9 for n. Same bug, just needs two more branches.
| return parseFloat(value) / 1000; | ||
| } | ||
|
|
||
| const groups = value.match(/(\d+(?:\.\d+)?)([BKMGTPEe])?(i)?(\d+)?/) || []; |
There was a problem hiding this comment.
This character class only matches uppercase 'K', but the canonical Kubernetes suffix is lowercase 'k' (unlike M/G/T/P/E which are uppercase) - util.ts's normalizeUnit checks endsWith('k') for exactly this reason. Worth fixing while you're in here, since it's the same 1000x-off bug for the one suffix that happens to be lowercase.
| expect(parseRam('0.5G')).toBe(500000000); | ||
| }); | ||
|
|
||
| it('should parse milli quantities (e.g. from prometheus-adapter)', () => { |
There was a problem hiding this comment.
These test parseRam directly, but the actual reported bug is a percentage calculation through divideK8sResources - there's already a describe block for that with a comparable CPU nano/micro case a bit further down that this could extend with a memory-milli one.
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
illume
left a comment
There was a problem hiding this comment.
Thanks for working on this.
Looks like there are git conflicts in this PR. Can you fix them up?
How to resolve conflicts
Rebase or merge the latest main into your branch, resolve the conflicts, and push the updated branch.
Summary
This PR fixes an issue where memory usage percentages are displayed approximately 1000× larger than expected when the Metrics API returns memory quantities in milli-units (for example,
2687146666666m).This occurs with prometheus-adapter, which commonly serves the
metrics.k8s.ioAPI in Prometheus-backed Kubernetes clusters that do not usemetrics-server.Related Issue
Fixes #6816
Changes
m(milli) suffix inparseUnitsOfBytes()(frontend/src/lib/units.ts).parseFloat(value) / 1000, matching the existing handling inparseCpu().1000m→12687146666666m→2687146666.6661.5m→0.0015Steps to Test
resourceRulesenabled) instead ofmetrics-server.Kiandmquantity formats.Screenshots
Before

Images are present in issue
After
Notes for Reviewers
N/A
Summary by CodeRabbit