Fixed memory leak caused by metrics remaining due to race conditions by halcyon-r · Pull Request #4822 · volcano-sh/volcano

halcyon-r · 2025-12-16T08:31:40Z

Verify that the job still exists before set job's metrics.

What type of PR is this?

/kind bug

What this PR does / why we need it:

Before writing the unschedule_task_count, job_share, and job_retry_counts metrics, check if the job exists to prevent leftover metrics.

Which issue(s) this PR fixes:

Fixes #4821

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

gemini-code-assist · 2025-12-16T08:31:59Z

Summary of Changes

Hello @halcyon-r, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a bug where scheduler metrics, specifically job_share, job_retry_counts, and unschedule_task_count, could continue to be updated or registered for jobs that had already terminated. This race condition led to the accumulation of stale metric data, effectively causing a memory leak. The solution introduces a new mechanism to verify a job's active status before any metric updates, ensuring that only relevant, existing jobs contribute to the reported metrics and maintaining a cleaner system state.

Highlights

New Job Termination Check: Introduced a new IsJobTerminated function within the scheduler cache to reliably determine if a job has been terminated or no longer exists, preventing operations on stale job data.
Metric Update Safeguards: Implemented conditional checks in the DRF (Dominant Resource Fairness) and Gang scheduling plugins to ensure that job_share, job_retry_counts, and unschedule_task_count metrics are only updated for active, non-terminated jobs.
API Exposure: The IsJobTerminated functionality is exposed through the Cache interface and the Session framework, making it accessible for consistent use across different scheduler components.
Memory Leak Fix: This change directly addresses a memory leak by preventing the accumulation of metric data for jobs that have already completed or been removed, ensuring cleaner state management and more accurate reporting.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request aims to fix a memory leak by preventing metric updates for terminated jobs. The approach of adding a check before updating metrics is correct and has been applied to drf and gang plugins. However, I've identified a critical data race in the new IsJobTerminated function that needs to be addressed to ensure thread safety.

pkg/scheduler/cache/cache.go

hajnalmt

Looks good to me, I had one question only.

Checked the occurancies too and the commit is fine (we don't update these metrics elsewehere which is surprising).

hajnalmt · 2025-12-17T07:54:20Z

pkg/scheduler/cache/interface.go

 	SharedDRAManager() framework.SharedDRAManager
+
+	// IsJobTerminated returns if the job was terminated
+	IsJobTerminated(jobId string) bool


Why haven't you used (jobId api.jobId) here too?
You would have spared the casting in cache.go

Yes, you are right. I‘ll optimize this part.

hajnalmt · 2025-12-17T08:15:52Z

pkg/scheduler/cache/cache.go

 }

+func (sc *SchedulerCache) IsJobTerminated(jobId string) bool {
+	sc.Mutex.Lock()


I have created an enhacement request for this as this should be a Read Lock on the long run.
See: #4824

Verify that the job still exists before set job's metrics. Signed-off-by: hairuiyang <hairuiyang@deeproute.ai>

hajnalmt

/lgtm

Thank you for the contribution and the optimization!

hzxuzhonghu

Good catch

@JesseStutler Should we backport to previous release

volcano-sh-bot · 2025-12-19T09:41:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hajnalmt, hzxuzhonghu

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:

~~pkg/scheduler/OWNERS~~ [hzxuzhonghu]

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

JesseStutler · 2025-12-19T12:23:59Z

@halcyon-r So it's the same issue here but with different solutions? #4760 I prefer to check if the job still exist rather than use ttl way, but also let @fengruotj takes a look

JesseStutler · 2025-12-19T12:25:55Z

pkg/scheduler/cache/interface.go

 	SharedDRAManager() framework.SharedDRAManager
+
+	// IsJobTerminated returns if the job was terminated
+	IsJobTerminated(jobId api.JobID) bool


I feel that we are too arbitrary to add a method to the cache interface here, the cache is not like the design that an interface should have

volcano-sh-bot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 16, 2025

volcano-sh-bot requested review from hudson741 and k82cn December 16, 2025 08:31

volcano-sh-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 16, 2025

gemini-code-assist bot reviewed Dec 16, 2025

View reviewed changes

pkg/scheduler/cache/cache.go Outdated Show resolved Hide resolved

halcyon-r force-pushed the master branch 3 times, most recently from d695610 to 9c29153 Compare December 16, 2025 08:50

hajnalmt reviewed Dec 17, 2025

View reviewed changes

Fixed memory leak caused by metrics remaining due to race conditions

83a9fba

Verify that the job still exists before set job's metrics. Signed-off-by: hairuiyang <hairuiyang@deeproute.ai>

halcyon-r force-pushed the master branch from 9c29153 to 83a9fba Compare December 17, 2025 08:41

hajnalmt approved these changes Dec 17, 2025

View reviewed changes

volcano-sh-bot assigned hajnalmt Dec 17, 2025

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2025

hzxuzhonghu approved these changes Dec 19, 2025

View reviewed changes

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 19, 2025

volcano-sh-bot merged commit 5a81d45 into volcano-sh:master Dec 19, 2025
22 checks passed

JesseStutler reviewed Dec 19, 2025

View reviewed changes

hajnalmt mentioned this pull request Jan 14, 2026

support ttl metrics for volcano-scheduler. #4760

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed memory leak caused by metrics remaining due to race conditions#4822

Fixed memory leak caused by metrics remaining due to race conditions#4822
volcano-sh-bot merged 1 commit intovolcano-sh:masterfrom
halcyon-r:master

halcyon-r commented Dec 16, 2025

Uh oh!

gemini-code-assist bot commented Dec 16, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

hajnalmt left a comment

Uh oh!

hajnalmt Dec 17, 2025

Uh oh!

halcyon-r Dec 17, 2025

Uh oh!

hajnalmt Dec 17, 2025

Uh oh!

hajnalmt left a comment

Uh oh!

hzxuzhonghu left a comment

Uh oh!

volcano-sh-bot commented Dec 19, 2025

Uh oh!

Uh oh!

JesseStutler commented Dec 19, 2025

Uh oh!

JesseStutler Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

halcyon-r commented Dec 16, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

gemini-code-assist bot commented Dec 16, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

hajnalmt left a comment

Choose a reason for hiding this comment

Uh oh!

hajnalmt Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

halcyon-r Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

hajnalmt Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

hajnalmt left a comment

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

volcano-sh-bot commented Dec 19, 2025

Uh oh!

Uh oh!

JesseStutler commented Dec 19, 2025

Uh oh!

JesseStutler Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants