-
Notifications
You must be signed in to change notification settings - Fork 946
fix: apply clientConnection QPS/burst to the manager client #3432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
cc9bbdb
332db30
8110cec
bf5ae42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ import ( | |
|
|
||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/runtime/serializer" | ||
| "k8s.io/client-go/rest" | ||
| ctrl "sigs.k8s.io/controller-runtime" | ||
| metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" | ||
| "sigs.k8s.io/controller-runtime/pkg/webhook" | ||
|
|
@@ -133,6 +134,19 @@ func Load(scheme *runtime.Scheme, configFile string, enableHTTP2 bool) (ctrl.Opt | |
| return options, cfg, nil | ||
| } | ||
|
|
||
| // ApplyClientConnection sets QPS and burst on the given rest.Config | ||
| // from the Configuration's clientConnection settings. | ||
| func ApplyClientConnection(restCfg *rest.Config, cfg *configapi.Configuration) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the rationale for having a separate function for this rather than doing this as part of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah - re-reading this I see the rest config is created in main.go directly. A separate function makes sense. |
||
| if cfg.ClientConnection != nil { | ||
| if cfg.ClientConnection.QPS != nil { | ||
| restCfg.QPS = *cfg.ClientConnection.QPS | ||
| } | ||
| if cfg.ClientConnection.Burst != nil { | ||
| restCfg.Burst = int(*cfg.ClientConnection.Burst) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // IsCertManagementEnabled returns true if certificate management is enabled. | ||
| // Returns true by default if not explicitly disabled. | ||
| func IsCertManagementEnabled(cfg *configapi.Configuration) bool { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -27,6 +27,7 @@ import ( | |||
| "github.com/google/go-cmp/cmp/cmpopts" | ||||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||
| "k8s.io/apimachinery/pkg/runtime" | ||||
| "k8s.io/client-go/rest" | ||||
| componentconfigv1alpha1 "k8s.io/component-base/config/v1alpha1" | ||||
| "k8s.io/utils/ptr" | ||||
| ctrl "sigs.k8s.io/controller-runtime" | ||||
|
|
@@ -798,6 +799,170 @@ func TestIsCertManagementEnabled(t *testing.T) { | |||
| } | ||||
| } | ||||
|
|
||||
| func TestApplyClientConnection(t *testing.T) { | ||||
| testcases := []struct { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the convention is to use map-based test cases rather than list - could you udpate pls? Line 130 in 7dfaa64
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @robert-bell , great catch 🙌
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done ✅ - Converted both TestApplyClientConnection and TestLoadAndApplyClientConnection to use map-based test cases.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @abhijeet-dhumal! |
||||
| name string | ||||
| cfg configapi.Configuration | ||||
| initialQPS float32 | ||||
| initialBurst int | ||||
| wantQPS float32 | ||||
| wantBurst int | ||||
| }{ | ||||
| { | ||||
| name: "nil ClientConnection keeps rest.Config defaults", | ||||
| cfg: configapi.Configuration{}, | ||||
| initialQPS: 5, | ||||
| initialBurst: 10, | ||||
| wantQPS: 5, | ||||
| wantBurst: 10, | ||||
| }, | ||||
| { | ||||
| name: "default values override client-go defaults", | ||||
| cfg: configapi.Configuration{ | ||||
| ClientConnection: &configapi.ClientConnection{ | ||||
| QPS: ptr.To[float32](50), | ||||
| Burst: ptr.To[int32](100), | ||||
| }, | ||||
| }, | ||||
| initialQPS: 5, | ||||
| initialBurst: 10, | ||||
| wantQPS: 50, | ||||
| wantBurst: 100, | ||||
| }, | ||||
| { | ||||
| name: "custom values applied", | ||||
| cfg: configapi.Configuration{ | ||||
| ClientConnection: &configapi.ClientConnection{ | ||||
| QPS: ptr.To[float32](200), | ||||
| Burst: ptr.To[int32](400), | ||||
| }, | ||||
| }, | ||||
| wantQPS: 200, | ||||
| wantBurst: 400, | ||||
| }, | ||||
| { | ||||
| name: "only QPS set preserves existing burst", | ||||
| cfg: configapi.Configuration{ | ||||
| ClientConnection: &configapi.ClientConnection{ | ||||
| QPS: ptr.To[float32](75), | ||||
| }, | ||||
| }, | ||||
| initialBurst: 10, | ||||
| wantQPS: 75, | ||||
| wantBurst: 10, | ||||
| }, | ||||
| { | ||||
| name: "only burst set preserves existing QPS", | ||||
| cfg: configapi.Configuration{ | ||||
| ClientConnection: &configapi.ClientConnection{ | ||||
| Burst: ptr.To[int32](150), | ||||
| }, | ||||
| }, | ||||
| initialQPS: 5, | ||||
| wantQPS: 5, | ||||
| wantBurst: 150, | ||||
| }, | ||||
| { | ||||
| name: "zero QPS is valid and applied", | ||||
| cfg: configapi.Configuration{ | ||||
| ClientConnection: &configapi.ClientConnection{ | ||||
| QPS: ptr.To[float32](0), | ||||
| Burst: ptr.To[int32](0), | ||||
| }, | ||||
| }, | ||||
| initialQPS: 5, | ||||
| initialBurst: 10, | ||||
| wantQPS: 0, | ||||
| wantBurst: 0, | ||||
| }, | ||||
| { | ||||
| name: "empty ClientConnection struct preserves existing values", | ||||
| cfg: configapi.Configuration{ | ||||
| ClientConnection: &configapi.ClientConnection{}, | ||||
| }, | ||||
| initialQPS: 5, | ||||
| initialBurst: 10, | ||||
| wantQPS: 5, | ||||
| wantBurst: 10, | ||||
| }, | ||||
| } | ||||
|
|
||||
| for _, tc := range testcases { | ||||
| t.Run(tc.name, func(t *testing.T) { | ||||
| restCfg := &rest.Config{ | ||||
| QPS: tc.initialQPS, | ||||
| Burst: tc.initialBurst, | ||||
| } | ||||
| ApplyClientConnection(restCfg, &tc.cfg) | ||||
| if restCfg.QPS != tc.wantQPS { | ||||
| t.Errorf("QPS = %v, want %v", restCfg.QPS, tc.wantQPS) | ||||
| } | ||||
| if restCfg.Burst != tc.wantBurst { | ||||
| t.Errorf("Burst = %v, want %v", restCfg.Burst, tc.wantBurst) | ||||
| } | ||||
| }) | ||||
| } | ||||
| } | ||||
|
|
||||
| func TestLoadAndApplyClientConnection(t *testing.T) { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this effectively re-testing logic we already cover in TestApplyClientConnection and TestLoad?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the review @robert-bell ! Addressed all your feedback ✅
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please review !
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @abhijeet-dhumal. I was more wondering whether this test is needed? |
||||
| testScheme := runtime.NewScheme() | ||||
| if err := configapi.AddToScheme(testScheme); err != nil { | ||||
| t.Fatal(err) | ||||
| } | ||||
|
|
||||
| tmpDir := t.TempDir() | ||||
|
|
||||
| customQPSConfig := filepath.Join(tmpDir, "custom-qps.yaml") | ||||
| if err := os.WriteFile(customQPSConfig, []byte(` | ||||
| apiVersion: config.trainer.kubeflow.org/v1alpha1 | ||||
| kind: Configuration | ||||
| clientConnection: | ||||
| qps: 100 | ||||
| burst: 200 | ||||
| `), os.FileMode(0600)); err != nil { | ||||
| t.Fatal(err) | ||||
| } | ||||
|
|
||||
| testcases := []struct { | ||||
| name string | ||||
| configFile string | ||||
| wantQPS float32 | ||||
| wantBurst int | ||||
| }{ | ||||
| { | ||||
| name: "default config applies default QPS/burst", | ||||
| configFile: "", | ||||
| wantQPS: 50, | ||||
| wantBurst: 100, | ||||
| }, | ||||
| { | ||||
| name: "custom config applies custom QPS/burst", | ||||
| configFile: customQPSConfig, | ||||
| wantQPS: 100, | ||||
| wantBurst: 200, | ||||
| }, | ||||
| } | ||||
|
|
||||
| for _, tc := range testcases { | ||||
| t.Run(tc.name, func(t *testing.T) { | ||||
| _, cfg, err := Load(testScheme, tc.configFile, false) | ||||
| if err != nil { | ||||
| t.Fatalf("Unexpected error: %v", err) | ||||
| } | ||||
|
|
||||
| restCfg := &rest.Config{QPS: 5, Burst: 10} | ||||
| ApplyClientConnection(restCfg, &cfg) | ||||
|
|
||||
| if restCfg.QPS != tc.wantQPS { | ||||
| t.Errorf("QPS = %v, want %v", restCfg.QPS, tc.wantQPS) | ||||
| } | ||||
| if restCfg.Burst != tc.wantBurst { | ||||
| t.Errorf("Burst = %v, want %v", restCfg.Burst, tc.wantBurst) | ||||
| } | ||||
| }) | ||||
| } | ||||
| } | ||||
|
|
||||
| func TestLoadHTTP2(t *testing.T) { | ||||
| testScheme := runtime.NewScheme() | ||||
| if err := configapi.AddToScheme(testScheme); err != nil { | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Helm's
defaulthere will treat explicit0as empty, so users cannot setclientConnection.qps/burstto 0 even though the API allows it; render the values directly (relying on values.yaml defaults) or gate on key existence (hasKey) instead ofdefaultso explicit 0 is preserved.