Skip to content

fix(device): use HasPrefix and TrimPrefix in DelQuota - #2272

Merged
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
SK8-infi:fix-quota
Aug 3, 2026
Merged

fix(device): use HasPrefix and TrimPrefix in DelQuota#2272
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
SK8-infi:fix-quota

Conversation

@SK8-infi

@SK8-infi SK8-infi commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind bug

What this PR does / why we need it:
Fixes an issue in QuotaManager.DelQuota where resource quota keys were not validated for the "limits." prefix before slicing.

Previously, DelQuota only checked if the key length was greater than len("limits.") (7 characters) and sliced the first 7 characters. As a result, any non-limits. key longer than 7 characters whose suffix after 7 matched a managed resource (e.g., 0123456nvidia.com/gpumem) would pass IsManagedQuota and reset quotaInfo.Limit = 0, unexpectedly corrupting namespace quota limits.

This PR updates DelQuota to use strings.HasPrefix(idx.String(), "limits.") and strings.TrimPrefix(idx.String(), "limits.") to be the same as AddQuota, ensuring non-limits. keys are safely ignored.

Which issue(s) this PR fixes:
Fixes #2271

Special notes for your reviewer:
This would be me first time contributing to HAMI. If anything is out of order or I made some mistake do let me know. Thanks.

Does this PR introduce a user-facing change?:
NA

Summary by CodeRabbit

  • Bug Fixes
    • Improved validation when removing device quota resources, preventing incorrectly formatted quota names from being processed.
    • Fixed an issue where removing quotas containing only unrelated resources could reset existing managed limits.
    • Existing managed memory limits now remain unchanged when unrelated quota resources are removed.

@hami-robot hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Aug 1, 2026
@hami-robot

hami-robot Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Welcome @SK8-infi! It looks like this is your first PR to Project-HAMi/HAMi 🎉

@hami-robot hami-robot Bot added the size/XS label Aug 1, 2026
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fee9fee4-7af3-4559-b2f9-ec6033d82a02

📥 Commits

Reviewing files that changed from the base of the PR and between 22fc0f4 and b01416f.

📒 Files selected for processing (2)
  • pkg/device/quota.go
  • pkg/device/quota_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/device/quota_test.go
  • pkg/device/quota.go

📝 Walkthrough

Walkthrough

DelQuota now validates the limits. prefix before extracting quota resource names. A regression test confirms that non-limits. keys do not reset active managed quota limits.

Changes

Quota validation

Layer / File(s) Summary
DelQuota prefix validation and regression coverage
pkg/device/quota.go, pkg/device/quota_test.go
DelQuota now uses strings.HasPrefix and strings.TrimPrefix to process only limits. quota keys. The test verifies that non-limits. keys preserve existing memory limits.

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

Suggested reviewers: chaunceyjiang

Poem

A rabbit checks each quota key,
“limits.” prefixes pass safely.
Other keys stay out of sight,
Active limits remain right.
Hop, hop—no sliced surprise!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary fix in DelQuota and identifies the HasPrefix and TrimPrefix changes.
Linked Issues check ✅ Passed The changes implement HasPrefix and prefix trimming in DelQuota and add a regression test for unrelated quota keys, satisfying issue #2271.
Out of Scope Changes check ✅ Passed The changes are limited to the DelQuota fix and its focused regression test, with no unrelated code changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@coderabbitai
coderabbitai Bot requested a review from mesutoezdil August 1, 2026 15:13

@Eshiv-Pandey Eshiv-Pandey 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.

i mean i dont see a functional behavior change here. IsManagedQuota() already appears to reject any malformed dn produced by the previous logic.
can you share a case where the old implementation behaved incorrectly? otherwise this seems more like a clean code kinda change than a bug fix

@SK8-infi

SK8-infi commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

i mean i dont see a functional behavior change here. IsManagedQuota() already appears to reject any malformed dn produced by the previous logic. can you share a case where the old implementation behaved incorrectly? otherwise this seems more like a clean code kinda change than a bug fix

@Eshiv-Pandey

If a key has a 7 char prefix before the resource name (thake this for ex... custom.nvidia.com/gpumem), stripping offset [7:] leaves the exact valid resource name (---> nvidia.com/gpumem). IsManagedQuota accepts it and accidentally wipes out the active quota limit for that namespace.
HasPrefix ensures keys without limits. are safely ignored.

Kind of maybe considered a very remote edge case but still better to be safe...do let me know if I am overlooking something.

@Eshiv-Pandey

Copy link
Copy Markdown
Contributor

makes sense. lgtm

@mesutoezdil mesutoezdil 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.

heads up, #2172 also touches quota.go and quota_test.go so watch for conflicts.

no ai assistance disclosure is present. if any ai tool was used, it must be disclosed per CONTRIBUTING.md: https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice

Comment thread pkg/device/quota.go
@SK8-infi

SK8-infi commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

heads up, #2172 also touches quota.go and quota_test.go so watch for conflicts.

no ai assistance disclosure is present. if any ai tool was used, it must be disclosed per CONTRIBUTING.md: https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice

#2172 seems to be closed for now....so ig we are good to go
Added the regression test

image

As for ai tool, will keep in mind to disclose prior next time. But was not used in this one due to minor fix.

@SK8-infi
SK8-infi requested a review from mesutoezdil August 2, 2026 18:21

@DSFans2014 DSFans2014 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@archlitchi

Copy link
Copy Markdown
Member

please fix the go-lint

Signed-off-by: SK8-infi <shivansh.katiyar1712@gmail.com>
@SK8-infi

SK8-infi commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@archlitchi fixed the linting issue. Will take care from now on...

@mesutoezdil

Copy link
Copy Markdown
Contributor

@archlitchi fixed the linting issue. Will take care from now on...

for future: pls refrain from force pushing once you have started getting reviews, it makes it much harder to tell what has changed from review to review.

@SK8-infi

SK8-infi commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@archlitchi fixed the linting issue. Will take care from now on...

for future: pls refrain from force pushing once you have started getting reviews, it makes it much harder to tell what has changed from review to review.

Okay sure. there was this one project I was working with..they say to keep one commit. So got confused

@Shouren Shouren left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@hami-robot hami-robot Bot added the lgtm label Aug 3, 2026
@hami-robot

hami-robot Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Shouren, SK8-infi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hami-robot hami-robot Bot added the approved label Aug 3, 2026
@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 65.90% <100.00%> (+1.67%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/device/quota.go 64.70% <100.00%> (+1.30%) ⬆️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hami-robot
hami-robot Bot merged commit a5251af into Project-HAMi:master Aug 3, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(quota): DelQuota uses length check instead of HasPrefix matching AddQuota

6 participants