fix: apply clientConnection QPS/burst to the manager client by abhijeet-dhumal · Pull Request #3432 · kubeflow/trainer

abhijeet-dhumal · 2026-04-16T07:25:02Z

What this PR does / why we need it:
The clientConnection QPS/burst configuration is validated and defaulted but never applied to the manager's Kubernetes client.

Fixes #3431

Extracts ApplyClientConnection() in pkg/config and calls it from main.go to wire QPS/burst onto rest.Config
Adds the missing clientConnection block to the Helm chart ConfigMap template and values.yaml
Adds unit and end-to-end config loading tests

Checklist:

[Docs] No doc changes needed - existing config field, just fixing the wiring.

Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>

google-oss-prow · 2026-04-16T07:25:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tenzen-y 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

Copilot

Pull request overview

This PR fixes a wiring gap where clientConnection QPS/burst settings (already defaulted/validated in the Configuration API) were not being applied to the controller manager’s Kubernetes rest.Config, and adds Helm chart parity plus tests to prevent regression.

Changes:

Adds ApplyClientConnection() in pkg/config and applies it before creating the controller-runtime manager.
Extends the Helm chart to emit clientConnection into the generated manager config.
Adds unit tests to cover applying clientConnection to a rest.Config, including default and custom config loading.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`pkg/config/config.go`	Introduces `ApplyClientConnection()` to apply QPS/burst onto a `rest.Config`.
`pkg/config/config_test.go`	Adds tests for `ApplyClientConnection()` and for load+apply behavior.
`cmd/trainer-controller-manager/main.go`	Wires `clientConnection` into the manager’s REST config before `ctrl.NewManager()`.
`charts/kubeflow-trainer/values.yaml`	Adds `manager.config.clientConnection` values (qps/burst) with defaults.
`charts/kubeflow-trainer/templates/manager/configmap.yaml`	Renders `clientConnection` into the manager configmap template.

Copilot · 2026-04-16T07:27:20Z

+      qps: {{ .Values.manager.config.clientConnection.qps | default 50 }}
+      burst: {{ .Values.manager.config.clientConnection.burst | default 100 }}


Using Helm's default here will treat explicit 0 as empty, so users cannot set clientConnection.qps/burst to 0 even though the API allows it; render the values directly (relying on values.yaml defaults) or gate on key existence (hasKey) instead of default so explicit 0 is preserved.

Suggested change

qps: {{ .Values.manager.config.clientConnection.qps | default 50 }}

burst: {{ .Values.manager.config.clientConnection.burst | default 100 }}

qps: {{ dig "manager" "config" "clientConnection" "qps" 50 .Values }}

burst: {{ dig "manager" "config" "clientConnection" "burst" 100 .Values }}

Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>

robert-bell

Thanks @abhijeet-dhumal this was a good spot.

robert-bell · 2026-04-21T15:43:59Z


+// ApplyClientConnection sets QPS and burst on the given rest.Config
+// from the Configuration's clientConnection settings.
+func ApplyClientConnection(restCfg *rest.Config, cfg *configapi.Configuration) {


what's the rationale for having a separate function for this rather than doing this as part of Load?

Ah - re-reading this I see the rest config is created in main.go directly. A separate function makes sense.

robert-bell · 2026-04-21T15:46:03Z

 }

+func TestApplyClientConnection(t *testing.T) {
+	testcases := []struct {


the convention is to use map-based test cases rather than list - could you udpate pls?

trainer/CONTRIBUTING.md

Line 130 in 7dfaa64

Define test cases as maps instead of slices to avoid dependencies on the running order. Map key should be equal to the test case name.

Thanks @robert-bell , great catch 🙌

Done ✅ - Converted both TestApplyClientConnection and TestLoadAndApplyClientConnection to use map-based test cases.

Thanks @abhijeet-dhumal!

robert-bell · 2026-04-23T15:22:52Z

+	}
+}
+
+func TestLoadAndApplyClientConnection(t *testing.T) {


Is this effectively re-testing logic we already cover in TestApplyClientConnection and TestLoad?

Thanks for the review @robert-bell ! Addressed all your feedback ✅

Please review !

Thanks @abhijeet-dhumal. I was more wondering whether this test is needed?

…convention Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>

Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>

fix: Apply clientConnection QPS/burst to the manager's REST client

cc9bbdb

Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>

Copilot AI review requested due to automatic review settings April 16, 2026 07:25

google-oss-prow Bot requested review from akshaychitneni and kuizhiqing April 16, 2026 07:25

google-oss-prow Bot added the size/L label Apr 16, 2026

Copilot AI reviewed Apr 16, 2026

View reviewed changes

abhijeet-dhumal mentioned this pull request Apr 16, 2026

clientConnection QPS/burst configuration is not applied to the manager client #3431

Open

abhijeet-dhumal changed the title ~~fix(config): Apply clientConnection QPS/burst to the manager client~~ fix(config): apply clientConnection QPS/burst to the manager client Apr 16, 2026

abhijeet-dhumal changed the title ~~fix(config): apply clientConnection QPS/burst to the manager client~~ fix: apply clientConnection QPS/burst to the manager client Apr 16, 2026

chore: Regenerate Helm README for clientConnection values

332db30

Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>

robert-bell reviewed Apr 21, 2026

View reviewed changes

robert-bell reviewed Apr 23, 2026

View reviewed changes

robert-bell mentioned this pull request Apr 23, 2026

fix(operator): apply Configuration.ClientConnection QPS/Burst to the manager rest.Config #3446

Closed

3 tasks

abhijeet-dhumal added 2 commits April 24, 2026 13:09

refactor: convert test cases from slices to maps per CONTRIBUTING.md …

8110cec

…convention Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>

fix: drop redundant default in Helm template and simplify smoke test

bf5ae42

Signed-off-by: abhijeet-dhumal <abhijeetdhumal652@gmail.com>

		qps: {{ .Values.manager.config.clientConnection.qps \| default 50 }}
		burst: {{ .Values.manager.config.clientConnection.burst \| default 100 }}

Conversation

abhijeet-dhumal commented Apr 16, 2026

Uh oh!

google-oss-prow Bot commented Apr 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

robert-bell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants