Skip to content

fix: return snapshot in InspectAllNodesUsage to avoid data race - #1967

Merged
hami-robot[bot] merged 2 commits into
Project-HAMi:masterfrom
DSFans2014:fix/overviewstatus
Jun 26, 2026
Merged

fix: return snapshot in InspectAllNodesUsage to avoid data race#1967
hami-robot[bot] merged 2 commits into
Project-HAMi:masterfrom
DSFans2014:fix/overviewstatus

Conversation

@DSFans2014

@DSFans2014 DSFans2014 commented Jun 23, 2026

Copy link
Copy Markdown
Member

What type of PR is this?

/kind bug

What this PR does / why we need it:

  • The metrics system reads the data returned by InspectAllNodesUsage. Since the underlying data may be concurrently modified, this could lead to a data race.
  • Remove the unused member cachedstatus from scheduler

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Signed-off-by: james <open4pd@4paradigm.com>
@hami-robot hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Jun 23, 2026
@hami-robot
hami-robot Bot requested review from archlitchi and wawa0210 June 23, 2026 11:07
@hami-robot hami-robot Bot added the approved label Jun 23, 2026
@github-actions github-actions Bot removed the kind/bug Something isn't working label Jun 23, 2026
@hami-robot hami-robot Bot added the size/S label Jun 23, 2026
@DSFans2014 DSFans2014 changed the title return snapshot in InspectAllNodesUsage to avoid data race fix: return snapshot in InspectAllNodesUsage to avoid data race Jun 23, 2026
@github-actions github-actions Bot added the kind/bug Something isn't working label Jun 23, 2026

@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 improves concurrency safety by switching to read locks in GetPod and creating a deep-copied snapshot under a read lock in InspectAllNodesUsage. However, a critical data race remains because s.overviewstatus is updated concurrently in getNodesUsage without holding the lock. It is recommended to refactor getNodesUsage to return the maps so that callers can safely assign them under the lock.

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/scheduler/scheduler.go
@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 65.00000% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/scheduler/scheduler.go 61.11% 7 Missing ⚠️
Flag Coverage Δ
unittests 59.04% <65.00%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
pkg/device/pods.go 62.67% <100.00%> (ø)
pkg/scheduler/scheduler.go 52.88% <61.11%> (+0.08%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: james <open4pd@4paradigm.com>
@mesutoezdil

Copy link
Copy Markdown
Contributor

second commit (Jun 24) fixes the race: getNodesUsage no longer writes s.overviewstatus directly. Filter discards the return, register assigns under s.lock.Lock(). RLock in InspectAllNodesUsage is sufficient.

@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 Jun 26, 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 6d2d19a into Project-HAMi:master Jun 26, 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