fix(scheduler): honour HAMI_NODELOCK_EXPIRE when the flag is not set - #2721
fix(scheduler): honour HAMI_NODELOCK_EXPIRE when the flag is not set#2721IIITManjeet wants to merge 2 commits into
Conversation
The nodelock package reads HAMI_NODELOCK_EXPIRE in its package init, but start() then assigned config.NodeLockTimeout over it unconditionally. That value holds the --node-lock-timeout default of 5m whenever the flag is absent, so the environment value was always discarded and the chart's scheduler.nodeLockExpire setting had no effect. Take the flag only when it was explicitly passed, leaving precedence as flag, then environment, then default. The flag state is read in RunE and passed into start(), because referring to rootCmd from start() makes Go report an initialization cycle for rootCmd. Signed-off-by: IIITManjeet <manjeetpathak2003@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: IIITManjeet The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe scheduler now uses ChangesNode-lock timeout handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change makes the configured node-lock expiration take effect while preserving explicit flag precedence; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
| if flagSet { | ||
| nodelock.NodeLockTimeout = config.NodeLockTimeout | ||
| return | ||
| } |
There was a problem hiding this comment.
1 alternative: make the flag default nodelock.NodeLockTimeout in init, the env is already applied there by import order, then no bool threading is needed. also nothing else reads config.NodeLockTimeout, is the write back needed?
There was a problem hiding this comment.
Good call on both. Done.
Flag default is now nodelock.NodeLockTimeout — main imports pkg/util/nodelock, so its init() (and setupNodeLockTimeout, which reads HAMI_NODELOCK_EXPIRE) completes before this package registers flags. The default already carries the env value, so start() goes back to the plain unconditional assignment and applyNodeLockTimeout, the bool param and the Changed(...) call are gone. The write-back went too — as you say, nothing outside cmd/scheduler reads config.NodeLockTimeout.
Side benefit: PrintPFlags runs before start(), so the startup log used to print FLAG: --node-lock-timeout="5m0s" while the effective value was 7s. They agree now.
Rebuilt at both revisions and compared startup logs — Set node lock timeout is unchanged in all four cases (env only 7s, env+flag 33s, neither 5m0s, bad env 5m0s).
One visible trade-off: --help now shows an env-dependent default (7s when the chart sets scheduler.nodeLockExpire: 7s). I think showing the real default is better, but happy to pin it back if you'd rather.
TestApplyNodeLockTimeout had no function left to cover, so it's now TestNodeLockTimeoutFlagDefault, asserting the registered default is nodelock.NodeLockTimeout — re-hardcoding a duration there is exactly how #2692 comes back. Env parsing is already covered in pkg/util/nodelock/nodelock_test.go.
Sourcing the --node-lock-timeout default from nodelock.NodeLockTimeout instead of a hard-coded 5m removes the need to inspect flag state at all. cmd/scheduler imports pkg/util/nodelock, so nodelock's init, and with it the read of HAMI_NODELOCK_EXPIRE, completes before this package registers its flags. The default therefore already carries the environment value and the assignment in start() is correct on its own, leaving precedence as flag, then environment, then default. This drops applyNodeLockTimeout, the bool parameter threaded through start(), and the write-back to config.NodeLockTimeout, which nothing outside cmd/scheduler reads. It also makes the startup flag log honest: flag.PrintPFlags runs before start(), so with HAMI_NODELOCK_EXPIRE=7s it reported 5m0s while the effective timeout was 7s. TestApplyNodeLockTimeout no longer has a function to cover and is replaced by TestNodeLockTimeoutFlagDefault, which asserts the registered default is nodelock.NodeLockTimeout. Parsing of the environment variable itself is already covered by pkg/util/nodelock. Suggested-by: mesutoezdil <mesutoezdil@users.noreply.github.com> Signed-off-by: IIITManjeet <manjeetpathak2003@gmail.com>
|
This is being closed because it does not comply with the contribution guidelines. |
|
Hey @mesutoezdil, apologies for breaking the contribution guidelines. Should I reframe it with my own wording for the reply and commit with a fresh PR or reopen it again with my updated reply. |
|
you MUST understand your codes, and should answer with your own words. |
|
yes @mesutoezdil agreed actually just asked llm to make it more professional like answer. Again, apologies for the llm drafted answer. If there is anything that I can help to undo my act, do let me know. |
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
The Helm chart exposes
scheduler.nodeLockExpire, which sets theHAMI_NODELOCK_EXPIREenvironment variable on the scheduler extender.
pkg/util/nodelockreads that variable inits package
init()and setsNodeLockTimeoutfrom it.start()then ran, unconditionally:config.NodeLockTimeoutis populated by the--node-lock-timeoutflag, which carries a5m default whether or not the flag is passed. The environment-derived value was therefore
always discarded a few milliseconds after being set, and
scheduler.nodeLockExpirehad noeffect at all. The chart's own default of
5mmasked this, so the setting only looksbroken once you change it.
This takes the flag only when it was explicitly provided, leaving precedence as flag, then
environment, then default.
Which issue(s) this PR fixes:
Fixes #2692
Special notes for your reviewer:
The issue suggests calling
rootCmd.Flags().Changed("node-lock-timeout")from insidestart(). That does not compile:rootCmd'sRunEclosure already refers tostart, sostartcannot refer back torootCmd. This PR reads the flag state inRunE, where cobra already supplies the*cobra.Command, and passes it intostart()as a parameter.I verified behaviour by building the scheduler binary from
masterand from this branchand reading its own startup log. "Set node lock timeout" is the effective value:
HAMI_NODELOCK_EXPIRE=7s, no flagHAMI_NODELOCK_EXPIRE=7s+--node-lock-timeout=33sHAMI_NODELOCK_EXPIRE=notadurationOnly the first row changes.
TestApplyNodeLockTimeoutcovers the three precedence paths.Per the hardware-validation gate in CONTRIBUTING, this change is scoped to the scheduler
extender and is covered by unit tests rather than GPU hardware.
Checks run locally (Go 1.26.6, linux/amd64):
go test -short --race ./pkg/... ./cmd/...— 37 packages ok, 0 failuresgolangci-lint run ./...(v2.12.2, matching CI)hack/verify-license.sh,hack/verify-import-aliases.sh,hack/verify-rbac.shgo mod tidy— no change togo.modorgo.sum#2692 also asks whether
HAMI_NODELOCK_EXPIREshould remain at all, or whether the chartshould set
--node-lock-timeoutinstead. I kept this PR to the precedence bug and left thechart untouched, since that is an interface decision for the maintainers. Happy to follow
up either way.
Does this PR introduce a user-facing change?:
AI assistance disclosure: I used AI assistance (Claude Code) throughout this change: to understand the nodelock init path and how config.NodeLockTimeout is populated, to find that the fix suggested in the issue does not compile, to implement applyNodeLockTimeout, and to write cmd/scheduler/nodelock_timeout_test.go. It also ran the before/after binary comparison and the local checks listed above, and drafted the commit message and this description. I reviewed the change, understand the initialization-cycle constraint that shapes it, and am able to answer questions about it directly.
Summary by CodeRabbit