From 640467ae4a933033de49aa6e2e557bc4e89a3bcb Mon Sep 17 00:00:00 2001 From: IIITManjeet Date: Wed, 19 Aug 2026 00:41:46 +0530 Subject: [PATCH 1/2] fix(scheduler): honour HAMI_NODELOCK_EXPIRE when the flag is not set 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 --- cmd/scheduler/main.go | 17 ++++++-- cmd/scheduler/nodelock_timeout_test.go | 59 ++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 4 deletions(-) create mode 100644 cmd/scheduler/nodelock_timeout_test.go diff --git a/cmd/scheduler/main.go b/cmd/scheduler/main.go index f7c68d1585..fa31f1abe7 100644 --- a/cmd/scheduler/main.go +++ b/cmd/scheduler/main.go @@ -54,7 +54,7 @@ var ( Short: "kubernetes vgpu scheduler", RunE: func(cmd *cobra.Command, args []string) error { flag.PrintPFlags(cmd.Flags()) - return start() + return start(cmd.Flags().Changed("node-lock-timeout")) }, } ) @@ -109,9 +109,18 @@ func injectProfilingRoute(router *httprouter.Router) { }) } -func start() error { - // Initialize node lock timeout from config - nodelock.NodeLockTimeout = config.NodeLockTimeout +// applyNodeLockTimeout applies the flag only when it was set explicitly, so it +// does not overwrite the value nodelock's init took from HAMI_NODELOCK_EXPIRE. +func applyNodeLockTimeout(flagSet bool) { + if flagSet { + nodelock.NodeLockTimeout = config.NodeLockTimeout + return + } + config.NodeLockTimeout = nodelock.NodeLockTimeout +} + +func start(nodeLockTimeoutFlagSet bool) error { + applyNodeLockTimeout(nodeLockTimeoutFlagSet) klog.InfoS("Set node lock timeout", "timeout", nodelock.NodeLockTimeout) client.InitGlobalClient( client.WithBurst(config.Burst), diff --git a/cmd/scheduler/nodelock_timeout_test.go b/cmd/scheduler/nodelock_timeout_test.go new file mode 100644 index 0000000000..a7b9b0c329 --- /dev/null +++ b/cmd/scheduler/nodelock_timeout_test.go @@ -0,0 +1,59 @@ +/* +Copyright 2024 The HAMi Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "testing" + "time" + + "github.com/Project-HAMi/HAMi/pkg/scheduler/config" + "github.com/Project-HAMi/HAMi/pkg/util/nodelock" +) + +func TestApplyNodeLockTimeout(t *testing.T) { + const defaultTimeout = 5 * time.Minute + + tests := []struct { + name string + flagSet bool + flag time.Duration + env time.Duration + want time.Duration + }{ + {name: "flag unset keeps env value", flagSet: false, flag: defaultTimeout, env: 5 * time.Second, want: 5 * time.Second}, + {name: "flag set overrides env", flagSet: true, flag: 5 * time.Second, env: defaultTimeout, want: 5 * time.Second}, + {name: "neither set keeps default", flagSet: false, flag: defaultTimeout, env: defaultTimeout, want: defaultTimeout}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + origFlag, origEnv := config.NodeLockTimeout, nodelock.NodeLockTimeout + t.Cleanup(func() { + config.NodeLockTimeout, nodelock.NodeLockTimeout = origFlag, origEnv + }) + config.NodeLockTimeout, nodelock.NodeLockTimeout = tc.flag, tc.env + + applyNodeLockTimeout(tc.flagSet) + + if nodelock.NodeLockTimeout != tc.want { + t.Errorf("nodelock.NodeLockTimeout = %v, want %v", nodelock.NodeLockTimeout, tc.want) + } + if config.NodeLockTimeout != tc.want { + t.Errorf("config.NodeLockTimeout = %v, want %v", config.NodeLockTimeout, tc.want) + } + }) + } +} From f2be4df29b9f936521f4aa62176a723d76d8ac96 Mon Sep 17 00:00:00 2001 From: IIITManjeet Date: Wed, 19 Aug 2026 14:45:15 +0530 Subject: [PATCH 2/2] fix(scheduler): take the node-lock-timeout default from nodelock 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 Signed-off-by: IIITManjeet --- cmd/scheduler/main.go | 21 ++++-------- cmd/scheduler/nodelock_timeout_test.go | 45 ++++++++------------------ 2 files changed, 21 insertions(+), 45 deletions(-) diff --git a/cmd/scheduler/main.go b/cmd/scheduler/main.go index fa31f1abe7..8da9f507fb 100644 --- a/cmd/scheduler/main.go +++ b/cmd/scheduler/main.go @@ -54,7 +54,7 @@ var ( Short: "kubernetes vgpu scheduler", RunE: func(cmd *cobra.Command, args []string) error { flag.PrintPFlags(cmd.Flags()) - return start(cmd.Flags().Changed("node-lock-timeout")) + return start() }, } ) @@ -76,7 +76,10 @@ func init() { rootCmd.Flags().IntVar(&config.Burst, "kube-burst", client.DefaultBurst, "Burst to use while talking with kube-apiserver.") rootCmd.Flags().IntVar(&config.Timeout, "kube-timeout", client.DefaultTimeout, "Timeout to use while talking with kube-apiserver.") rootCmd.Flags().BoolVar(&enableProfiling, "profiling", false, "Enable pprof profiling via HTTP server") - rootCmd.Flags().DurationVar(&config.NodeLockTimeout, "node-lock-timeout", time.Minute*5, "timeout for node locks") + // nodelock's package init runs before this one and has already applied + // HAMI_NODELOCK_EXPIRE, so taking its value as the flag default leaves + // precedence as flag, then environment, then default. + rootCmd.Flags().DurationVar(&config.NodeLockTimeout, "node-lock-timeout", nodelock.NodeLockTimeout, "timeout for node locks") rootCmd.Flags().DurationVar(&config.NodeLockRetryTimeout, "node-lock-retry-timeout", 28*time.Second, "timeout for retrying LockNode when contended by another PodGroup member (0 disables retry). Align the Extender's httpTimeout in KubeSchedulerConfiguration with this value.") rootCmd.Flags().BoolVar(&config.ForceOverwriteDefaultScheduler, "force-overwrite-default-scheduler", true, "Overwrite schedulerName in Pod Spec when set to the const DefaultSchedulerName in https://k8s.io/api/core/v1 package") @@ -109,18 +112,8 @@ func injectProfilingRoute(router *httprouter.Router) { }) } -// applyNodeLockTimeout applies the flag only when it was set explicitly, so it -// does not overwrite the value nodelock's init took from HAMI_NODELOCK_EXPIRE. -func applyNodeLockTimeout(flagSet bool) { - if flagSet { - nodelock.NodeLockTimeout = config.NodeLockTimeout - return - } - config.NodeLockTimeout = nodelock.NodeLockTimeout -} - -func start(nodeLockTimeoutFlagSet bool) error { - applyNodeLockTimeout(nodeLockTimeoutFlagSet) +func start() error { + nodelock.NodeLockTimeout = config.NodeLockTimeout klog.InfoS("Set node lock timeout", "timeout", nodelock.NodeLockTimeout) client.InitGlobalClient( client.WithBurst(config.Burst), diff --git a/cmd/scheduler/nodelock_timeout_test.go b/cmd/scheduler/nodelock_timeout_test.go index a7b9b0c329..bedfa24f64 100644 --- a/cmd/scheduler/nodelock_timeout_test.go +++ b/cmd/scheduler/nodelock_timeout_test.go @@ -18,42 +18,25 @@ package main import ( "testing" - "time" "github.com/Project-HAMi/HAMi/pkg/scheduler/config" "github.com/Project-HAMi/HAMi/pkg/util/nodelock" ) -func TestApplyNodeLockTimeout(t *testing.T) { - const defaultTimeout = 5 * time.Minute - - tests := []struct { - name string - flagSet bool - flag time.Duration - env time.Duration - want time.Duration - }{ - {name: "flag unset keeps env value", flagSet: false, flag: defaultTimeout, env: 5 * time.Second, want: 5 * time.Second}, - {name: "flag set overrides env", flagSet: true, flag: 5 * time.Second, env: defaultTimeout, want: 5 * time.Second}, - {name: "neither set keeps default", flagSet: false, flag: defaultTimeout, env: defaultTimeout, want: defaultTimeout}, +// TestNodeLockTimeoutFlagDefault guards the precedence of flag, then +// environment, then default. nodelock's init applies HAMI_NODELOCK_EXPIRE +// before this package's init registers flags, so the flag default must be +// nodelock.NodeLockTimeout. Hard-coding a duration here instead would discard +// the environment value on every start, which is the bug in #2692. +func TestNodeLockTimeoutFlagDefault(t *testing.T) { + f := rootCmd.Flags().Lookup("node-lock-timeout") + if f == nil { + t.Fatal("node-lock-timeout flag is not registered") } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - origFlag, origEnv := config.NodeLockTimeout, nodelock.NodeLockTimeout - t.Cleanup(func() { - config.NodeLockTimeout, nodelock.NodeLockTimeout = origFlag, origEnv - }) - config.NodeLockTimeout, nodelock.NodeLockTimeout = tc.flag, tc.env - - applyNodeLockTimeout(tc.flagSet) - - if nodelock.NodeLockTimeout != tc.want { - t.Errorf("nodelock.NodeLockTimeout = %v, want %v", nodelock.NodeLockTimeout, tc.want) - } - if config.NodeLockTimeout != tc.want { - t.Errorf("config.NodeLockTimeout = %v, want %v", config.NodeLockTimeout, tc.want) - } - }) + if want := nodelock.NodeLockTimeout.String(); f.DefValue != want { + t.Errorf("node-lock-timeout default = %s, want %s (nodelock.NodeLockTimeout)", f.DefValue, want) + } + if config.NodeLockTimeout != nodelock.NodeLockTimeout { + t.Errorf("config.NodeLockTimeout = %v, want %v", config.NodeLockTimeout, nodelock.NodeLockTimeout) } }