From 9e38ce69aa5ca5cd91dd948a73cefcd681ef34df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kuba=20Tu=C5=BCnik?= <jtuznik@google.com>
Date: Tue, 10 Jun 2025 16:54:43 +0200
Subject: [PATCH] Fix parsing --drain-priority-config
The --drain-priority-config flag was only parsed if isFlagPassed()
returned true for it. However, isFlagPassed() would actually silently
never work. The implementation relied on walking the flags parsed by
the standard Go "flag" pkg. This seems like it would work since CA
defines its flags using the standard "flag" pkg. However, the flags
are then actually parsed and processed by the "github.com/spf13/pflag"
pkg, so isFlagPassed() could never see them.
This commit replaces removes isFlagPassed() and replaces the calls
with a pkg-provided pflag.CommandLine.Changed(). Unit tests are added
to verify that the flag is correctly parsed after this change.
---
cluster-autoscaler/config/flags/flags.go | 16 ++-----
cluster-autoscaler/config/flags/flags_test.go | 48 +++++++++++++++++++
2 files changed, 51 insertions(+), 13 deletions(-)
diff --git a/cluster-autoscaler/config/flags/flags.go b/cluster-autoscaler/config/flags/flags.go
index 11533a0822..8012fde264 100644
--- a/cluster-autoscaler/config/flags/flags.go
+++ b/cluster-autoscaler/config/flags/flags.go
@@ -34,7 +34,7 @@ import (
"k8s.io/autoscaler/cluster-autoscaler/utils/units"
"k8s.io/client-go/rest"
- klog "k8s.io/klog/v2"
+ "k8s.io/klog/v2"
kubelet_config "k8s.io/kubernetes/pkg/kubelet/apis/config"
scheduler_config "k8s.io/kubernetes/pkg/scheduler/apis/config"
)
@@ -269,12 +269,12 @@ func createAutoscalingOptions() config.AutoscalingOptions {
klog.Fatalf("Failed to get scheduler config: %v", err)
}
- if isFlagPassed("drain-priority-config") && isFlagPassed("max-graceful-termination-sec") {
+ if pflag.CommandLine.Changed("drain-priority-config") && pflag.CommandLine.Changed("max-graceful-termination-sec") {
klog.Fatalf("Invalid configuration, could not use --drain-priority-config together with --max-graceful-termination-sec")
}
var drainPriorityConfigMap []kubelet_config.ShutdownGracePeriodByPodPriority
- if isFlagPassed("drain-priority-config") {
+ if pflag.CommandLine.Changed("drain-priority-config") {
drainPriorityConfigMap = parseShutdownGracePeriodsAndPriorities(*drainPriorityConfig)
if len(drainPriorityConfigMap) == 0 {
klog.Fatalf("Invalid configuration, parsing --drain-priority-config")
@@ -409,16 +409,6 @@ func createAutoscalingOptions() config.AutoscalingOptions {
}
}
-func isFlagPassed(name string) bool {
- found := false
- flag.Visit(func(f *flag.Flag) {
- if f.Name == name {
- found = true
- }
- })
- return found
-}
-
func minMaxFlagString(min, max int64) string {
return fmt.Sprintf("%v:%v", min, max)
}
diff --git a/cluster-autoscaler/config/flags/flags_test.go b/cluster-autoscaler/config/flags/flags_test.go
index a10678426b..3851a9a5da 100644
--- a/cluster-autoscaler/config/flags/flags_test.go
+++ b/cluster-autoscaler/config/flags/flags_test.go
@@ -17,11 +17,15 @@ limitations under the License.
package flags
import (
+ "flag"
"testing"
"k8s.io/autoscaler/cluster-autoscaler/config"
kubelet_config "k8s.io/kubernetes/pkg/kubelet/apis/config"
+ "github.com/google/go-cmp/cmp"
+ "github.com/google/go-cmp/cmp/cmpopts"
+ "github.com/spf13/pflag"
"github.com/stretchr/testify/assert"
)
@@ -146,3 +150,47 @@ func TestParseShutdownGracePeriodsAndPriorities(t *testing.T) {
})
}
}
+
+func TestCreateAutoscalingOptions(t *testing.T) {
+ for _, tc := range []struct {
+ testName string
+ flags []string
+ wantOptionsAsserter func(t *testing.T, gotOptions config.AutoscalingOptions)
+ }{
+ {
+ testName: "DrainPriorityConfig defaults to an empty list when the flag isn't passed",
+ flags: []string{},
+ wantOptionsAsserter: func(t *testing.T, gotOptions config.AutoscalingOptions) {
+ if diff := cmp.Diff([]kubelet_config.ShutdownGracePeriodByPodPriority{}, gotOptions.DrainPriorityConfig, cmpopts.EquateEmpty()); diff != "" {
+ t.Errorf("createAutoscalingOptions(): unexpected DrainPriorityConfig field (-want +got): %s", diff)
+ }
+ },
+ },
+ {
+ testName: "DrainPriorityConfig is parsed correctly when the flag passed",
+ flags: []string{"--drain-priority-config", "5000:60,3000:50,0:40"},
+ wantOptionsAsserter: func(t *testing.T, gotOptions config.AutoscalingOptions) {
+ wantConfig := []kubelet_config.ShutdownGracePeriodByPodPriority{
+ {Priority: 5000, ShutdownGracePeriodSeconds: 60},
+ {Priority: 3000, ShutdownGracePeriodSeconds: 50},
+ {Priority: 0, ShutdownGracePeriodSeconds: 40},
+ }
+ if diff := cmp.Diff(wantConfig, gotOptions.DrainPriorityConfig); diff != "" {
+ t.Errorf("createAutoscalingOptions(): unexpected DrainPriorityConfig field (-want +got): %s", diff)
+ }
+ },
+ },
+ } {
+ t.Run(tc.testName, func(t *testing.T) {
+ pflag.CommandLine = pflag.NewFlagSet("test", pflag.ExitOnError)
+ pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
+ err := pflag.CommandLine.Parse(tc.flags)
+ if err != nil {
+ t.Errorf("pflag.CommandLine.Parse() got unexpected error: %v", err)
+ }
+
+ gotOptions := createAutoscalingOptions()
+ tc.wantOptionsAsserter(t, gotOptions)
+ })
+ }
+}
--
GitLab