Skip to content

fix: skip owner check for legacy lock format - #2054

Merged
hami-robot[bot] merged 2 commits into
Project-HAMi:masterfrom
DSFans2014:feat/node-lock-legacy-format
Jul 13, 2026
Merged

fix: skip owner check for legacy lock format#2054
hami-robot[bot] merged 2 commits into
Project-HAMi:masterfrom
DSFans2014:feat/node-lock-legacy-format

Conversation

@DSFans2014

@DSFans2014 DSFans2014 commented Jul 10, 2026

Copy link
Copy Markdown
Member

What type of PR is this?

/kind bug

What this PR does / why we need it:

When using HAMi vNPU with Volcano and enabling node lock, the node lock value stored in Volcano is a timestamp. However, the ReleaseNodeLock function called in ascend-device-plugin expects the lock value to contain the pod's namespace and name. Since the timestamp format does not match the lock is never released.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Summary by CodeRabbit

  • Bug Fixes
    • Fixed node lock release handling for legacy timestamp-only annotation values, allowing locks to be released when the stored format is older than the current one.
  • Tests
    • Expanded node lock release test coverage with new cases for legacy-format locks and additional post-release verification that the lock annotation is removed.

@hami-robot hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Jul 10, 2026
@hami-robot
hami-robot Bot requested review from archlitchi and mesutoezdil July 10, 2026 09:21
@hami-robot hami-robot Bot added the approved label Jul 10, 2026
@github-actions github-actions Bot added kind/feature new function and removed kind/bug Something isn't working labels Jul 10, 2026
@DSFans2014 DSFans2014 changed the title feat: skip owner check for legacy lock format fix: skip owner check for legacy lock format Jul 10, 2026
@hami-robot hami-robot Bot added the size/S label Jul 10, 2026
@github-actions github-actions Bot added kind/bug Something isn't working and removed kind/feature new function labels Jul 10, 2026
@coderabbitai

coderabbitai Bot commented Jul 10, 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: 6d48cebb-d1d7-4dd3-8880-b1e74b36986d

📥 Commits

Reviewing files that changed from the base of the PR and between 62a0bbf and dc7d08f.

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

📝 Walkthrough

Walkthrough

ReleaseNodeLock now releases legacy timestamp-only node-lock annotations while retaining ownership checks for current formatted values. TestReleaseNodeLock verifies annotation removal for successful releases, including the legacy format.

Changes

Node Lock Release

Layer / File(s) Summary
Legacy lock release compatibility
pkg/util/nodelock/nodelock.go, pkg/util/nodelock/nodelock_test.go
ReleaseNodeLock bypasses the suffix-based ownership check for timestamp-only legacy values, while tests verify successful release and removal of the lock annotation.

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

Poem

A rabbit found an old lock in the hay,
Its timestamp still marked the way.
“No suffix?” it cheered,
The lock disappeared,
And tests hopped happily away.

🚥 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 accurately summarizes the main change: bypassing the owner check for legacy node-lock format.
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

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.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces backward compatibility for the legacy node lock format (a simple timestamp) by checking if the lock string contains the separator before validating the owner. A corresponding unit test has been added to verify this behavior. The reviewer suggested improving performance and readability by replacing fmt.Sprintf with direct string concatenation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread pkg/util/nodelock/nodelock.go Outdated
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 59.66% <100.00%> (ø)

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

Files with missing lines Coverage Δ
pkg/util/nodelock/nodelock.go 68.18% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread pkg/util/nodelock/nodelock_test.go
@mesutoezdil

Copy link
Copy Markdown
Contributor

first commit message uses feat: but this is a bug fix, it should be fix: to match the pr title and conventional commits.

@DSFans2014
DSFans2014 force-pushed the feat/node-lock-legacy-format branch from adb5a72 to b018f4a Compare July 10, 2026 10:22
@hami-robot hami-robot Bot added size/M and removed size/S labels Jul 10, 2026
Signed-off-by: james <open4pd@4paradigm.com>
@DSFans2014
DSFans2014 force-pushed the feat/node-lock-legacy-format branch from b018f4a to 62a0bbf Compare July 10, 2026 10:25
Signed-off-by: james <open4pd@4paradigm.com>
@DSFans2014
DSFans2014 force-pushed the feat/node-lock-legacy-format branch from 62a0bbf to dc7d08f Compare July 10, 2026 10:26
@mesutoezdil

Copy link
Copy Markdown
Contributor

/lgtm

@archlitchi archlitchi 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 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, DSFans2014

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:
  • OWNERS [DSFans2014,archlitchi]

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 merged commit 5f0adaa into Project-HAMi:master Jul 13, 2026
15 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.

3 participants