fix(scheduler): respect HAMI_NODELOCK_EXPIRE env when --node-lock-timeout flag is absent by Siraryansingh · Pull Request #2743 · Project-HAMi/HAMi

Siraryansingh · 2026-08-20T07:49:12Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

The Helm chart has scheduler.nodeLockExpire which sets HAMI_NODELOCK_EXPIRE env var on the scheduler pod. The nodelock package reads this in init() and sets NodeLockTimeout correctly. But then in start(), we do:

nodelock.NodeLockTimeout = config.NodeLockTimeout

config.NodeLockTimeout comes from the --node-lock-timeout cobra flag which defaults to 5m. Since cobra always fills in the default even when the flag isn't passed, this line overwrites whatever the env var set. So scheduler.nodeLockExpire in the Helm chart literally does nothing.

I used cmd.Flags().Changed("node-lock-timeout") to check if the flag was actually passed by the user. If it was, use the flag value. If not, keep whatever init() set from the env var.

After this fix the precedence is:

--node-lock-timeout flag (if explicitly passed)
HAMI_NODELOCK_EXPIRE env var
default 5m

Fixes #2692

AI Disclosure: I used Gemini to help explore the codebase and understand the cobra flag flow, but I identified the root cause myself by tracing through nodelock.go init -> main.go start. The fix and testing were done with AI assistance.

How was this tested?

go build ./cmd/scheduler/... passes
go test ./pkg/util/nodelock/... -run TestSetupNodeLockTimeout passes
all existing nodelock tests still pass

Summary by CodeRabbit

Bug Fixes
- Improved scheduler handling of the node-lock timeout setting.
- Explicit command-line values now take precedence, while environment-provided and default settings are preserved when no flag is specified.
- Added logging to indicate which timeout source was selected.
Tests
- Added coverage for default, environment-derived, and explicitly configured timeout behavior.

hami-robot · 2026-08-20T07:49:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Siraryansingh
Once this PR has been reviewed and has the lgtm label, please assign shouren for approval. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

coderabbitai · 2026-08-20T07:50:07Z

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: 11161fb8-dffe-4ae3-b1db-4f8fbb3d08e6

📥 Commits

Reviewing files that changed from the base of the PR and between bb59ec4 and f3a4f92.

📒 Files selected for processing (2)

cmd/scheduler/main.go
cmd/scheduler/main_test.go

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

The scheduler passes its Cobra command to start. It applies --node-lock-timeout only when explicitly provided. Otherwise, it preserves the environment-derived or default timeout and tests each behavior.

Changes

Scheduler timeout precedence

Layer / File(s)	Summary
Conditional node-lock timeout selection `cmd/scheduler/main.go`, `cmd/scheduler/main_test.go`	The command callback passes the Cobra command to `start`. `start` updates `nodelock.NodeLockTimeout` only when the flag was provided. Tests cover default, environment-derived, and explicit flag values.

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

Merge Risk: ⚪ Minimal · up to f3a4f

The scheduler now honors the explicit node-lock timeout flag before the environment setting and default, with no actionable merge-blocking risk remaining after normal checks and review.

Possibly related PRs

Project-HAMi/HAMi#2722: Both changes preserve the environment-derived timeout unless the flag is explicit.
Project-HAMi/HAMi#2721: Both changes adjust node-lock timeout precedence.

Suggested reviewers: archlitchi

Poem

A rabbit guards the timeout gate,
The explicit flag decides its fate.
If absent, environment values stay,
And defaults no longer hop away.
The scheduler logs the choice,
While burrows cheer its steady voice!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.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 scheduler fix that preserves HAMI_NODELOCK_EXPIRE when the CLI flag is absent.
Linked Issues check	✅ Passed	The changes implement the required precedence: explicit flag, environment variable, then the built-in default, with regression tests for all cases [`#2692`].
Out of Scope Changes check	✅ Passed	The changes are limited to scheduler timeout resolution and regression tests required by the linked issue [`#2692`].

✨ 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.}

Siraryansingh · 2026-08-20T07:57:18Z

hey, just wanted to give some context on how i found this.

i was going through the open issues and #2692 caught my eye. i traced the flow from the helm chart (values.yaml -> deployment template -> env var) into the go code and noticed the problem pretty quickly - nodelock.init() reads the env var fine, but then start() just stomps on it with the cobra flag default.

the fix is small - just pass the *cobra.Command into start() and check cmd.Flags().Changed() before overwriting. this is the standard cobra pattern for "flag vs env var" precedence.

i saw that #2721 and #2722 tried to fix the same issue. i came across it independently while looking through open bugs - happy to answer any questions about the approach or the code.

also adding an AI disclosure since the guidelines require it: i used gemini to help navigate the codebase and understand the flag registration flow, but the root cause analysis and fix were done by me with AI assistance. i can explain any part of the change if needed.

mesutoezdil · 2026-08-20T10:38:22Z

+	// explicitly passed on the command line. When the flag is absent, the value
+	// parsed from the HAMI_NODELOCK_EXPIRE environment variable (set by the
+	// Helm chart's scheduler.nodeLockExpire) in nodelock.init() is preserved.
+	if cmd.Flags().Changed("node-lock-timeout") {


no test for the fix, the listed nodelock test does not cover this branch. extract it into a helper taking a bool and add a regression test for flag set, env only, neither.

codecov · 2026-08-20T10:44:21Z

Codecov Report

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

Files with missing lines	Patch %	Lines
cmd/scheduler/main.go	0.00%	7 Missing ⚠️

Flag	Coverage Δ
unittests	`63.15% <0.00%> (+0.50%)`	⬆️

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

Files with missing lines	Coverage Δ
cmd/scheduler/main.go	`22.32% <0.00%> (-0.62%)`	⬇️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…-timeout flag is not passed The --node-lock-timeout flag's default value (5m) unconditionally overwrites the NodeLockTimeout set by nodelock.init() from the HAMI_NODELOCK_EXPIRE environment variable. This makes the Helm chart's scheduler.nodeLockExpire setting completely ineffective — the timeout is always 5 minutes regardless of the env var value. Use cobra's Flags().Changed() to detect whether the user explicitly passed --node-lock-timeout on the command line. When the flag is absent, preserve the value already parsed from HAMI_NODELOCK_EXPIRE. Precedence after this fix: 1. --node-lock-timeout flag (highest, explicit CLI) 2. HAMI_NODELOCK_EXPIRE env var (Helm chart / container env) 3. Built-in default of 5 minutes (lowest) Fixes Project-HAMi#2692 Signed-off-by: Aryan Singh <aryansingh.as1012@gmail.com>

Siraryansingh · 2026-08-20T11:42:15Z

@mesutoezdil thanks for the review.

I extracted the resolution logic into resolveNodeLockTimeout(flagChanged bool) in cmd/scheduler/main.go and added a regression test in cmd/scheduler/main_test.go covering all three cases:

neither flag nor env set - preserves default (5m)
env set, flag not set - preserves env setting (10m)
flag explicitly set - overrides with flag value (2m)

All tests in cmd/scheduler pass cleanly!

mesutoezdil · 2026-08-21T05:53:55Z

This is being closed because it does not comply with the contribution guidelines.

hami-robot Bot requested review from archlitchi and lengrongfu August 20, 2026 07:49

github-actions Bot added the kind/bug Something isn't working label Aug 20, 2026

mesutoezdil requested changes Aug 20, 2026

View reviewed changes

Siraryansingh temporarily deployed to nvidia August 20, 2026 10:48 — with GitHub Actions Inactive

Siraryansingh force-pushed the fix/nodelock-expire-env-override branch from bb59ec4 to f3a4f92 Compare August 20, 2026 11:42

mesutoezdil closed this Aug 21, 2026

Navigation Menu

Uh oh!

fix(scheduler): respect HAMI_NODELOCK_EXPIRE env when --node-lock-timeout flag is absent - #2743

fix(scheduler): respect HAMI_NODELOCK_EXPIRE env when --node-lock-timeout flag is absent#2743
Siraryansingh wants to merge 1 commit into
Project-HAMi:masterfrom
Siraryansingh:fix/nodelock-expire-env-override

Siraryansingh commented Aug 20, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

hami-robot Bot commented Aug 20, 2026

Uh oh!

coderabbitai Bot commented Aug 20, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Siraryansingh commented Aug 20, 2026

Uh oh!

mesutoezdil Aug 20, 2026

Uh oh!

codecov Bot commented Aug 20, 2026

Uh oh!

Siraryansingh commented Aug 20, 2026 •

edited

Loading

Uh oh!

mesutoezdil commented Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Siraryansingh commented Aug 20, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

hami-robot Bot commented Aug 20, 2026

Uh oh!

coderabbitai Bot commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Siraryansingh commented Aug 20, 2026

Uh oh!

mesutoezdil Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 20, 2026

Codecov Report

Uh oh!

Siraryansingh commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mesutoezdil commented Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Siraryansingh commented Aug 20, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 20, 2026 •

edited

Loading

Siraryansingh commented Aug 20, 2026 •

edited

Loading