Config API for Kubeflow Trainer controller manager #2428

chahatsagarmain · 2025-02-09T16:08:16Z

What this PR does / why we need it:

Adds Config api for flags defined for traininig operator

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #2420

Checklist:

Docs included if any changes are user facing

Signed-off-by: chahatsagarmain <[email protected]>

google-oss-prow · 2025-02-09T16:08:32Z

[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 jeffwan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

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

Signed-off-by: chahatsagarmain <[email protected]>

andreyvelich

Thank you for this @chahatsagarmain!
/assign @Electronic-Waste @kubeflow/wg-training-leads @astefanutti Please can you also take a look ?

andreyvelich · 2025-02-24T17:53:00Z

pkg/apis/trainer/v1alpha1/types.go

@@ -0,0 +1,50 @@
+// Copyright 2024 The Kubeflow Authors


Can we have dedicate directory for Config APIs and place it in /apis/config/v1alpha1/configuration_types.go ?
Similar to Kueue: https://github.com/kubernetes-sigs/kueue/tree/main/apis/config/v1beta1

andreyvelich · 2025-02-24T17:59:31Z

pkg/apis/trainer/v1alpha1/types.go

+)
+
+// TrainerControllerConfig represents the configuration for the training operator.
+type TrainerControllerConfig struct {


Let's call it type Configuration struct

Suggested change

type TrainerControllerConfig struct {

type Configuration struct {

andreyvelich · 2025-02-24T18:00:09Z

pkg/apis/trainer/v1alpha1/types.go

+
+	ControllerManager *ControllerManager `json:"controllerManager,omitempty"`
+	CertGeneration   *CertGeneration   `json:"certGeneration,omitempty"`
+}


You might need to fix Go formatting here.

Try to run make fmt in the terminal, just FYI @chahatsagarmain :)

andreyvelich · 2025-02-24T18:06:06Z

pkg/apis/trainer/v1alpha1/types.go

+	WebhookServerPort  int    `json:"webhookServerPort,omitempty"`
+	WebhookServiceName string `json:"webhookServiceName,omitempty"`
+	WebhookSecretName  string `json:"webhookSecretName,omitempty"`


We might need to explain what does each value mean and add the Defaults values:
https://github.com/kubeflow/katib/blob/master/pkg/apis/config/v1beta1/types.go#L55

andreyvelich · 2025-02-24T18:06:43Z

pkg/apis/trainer/v1alpha1/types.go

+}
+
+// CertGeneration holds configuration related to webhook server certificate generation.
+type CertGeneration struct {


Should we also have flag to enable/disable Cert Generator ?

andreyvelich · 2025-02-24T18:08:43Z

pkg/apis/trainer/v1alpha1/types.go

+type ControllerManager struct {
+	MetricsBindAddress      string `json:"metricsBindAddress,omitempty"`
+	HealthProbeBindAddress  string `json:"healthProbeBindAddress,omitempty"`
+	LeaderElect             bool   `json:"leaderElect,omitempty"`
+	MetricsSecure           bool   `json:"metricsSecure,omitempty"`
+	EnableHTTP2             bool   `json:"enableHttp2,omitempty"`
+}


We might want to split these values, similar to Kueue: https://github.com/kubernetes-sigs/kueue/blob/main/apis/config/v1beta1/configuration_types.go#L110-L125.

andreyvelich · 2025-02-24T18:10:33Z

cmd/trainer-controller-manager/main.go

 		"Use :8443 for HTTPS or :8080 for HTTP, or leave as 0 to disable the metrics service.")
-	flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
-	flag.BoolVar(&enableLeaderElection, "leader-elect", false,
+	flag.StringVar(&trainerCfg.ControllerManager.HealthProbeBindAddress, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")


I think, you should read this config from the file, similar to this: https://github.com/kubernetes-sigs/jobset/blob/main/main.go#L84C18-L87
We might also want to have the default config which enables the internal Cert Generator:
https://github.com/kubernetes-sigs/jobset/blob/main/config/components/manager/controller_manager_config.yaml#L2-L3

andreyvelich · 2025-02-24T18:11:12Z

/ok-to-test

andreyvelich · 2025-02-24T18:12:02Z

Here is an example on how to validate and read config: https://github.com/kubernetes-sigs/jobset/tree/main/pkg/config

Electronic-Waste

@chahatsagarmain Thanks for this. I also have some comments for you:)

Electronic-Waste · 2025-02-25T02:35:23Z

cmd/trainer-controller-manager/main.go

-	flag.IntVar(&webhookServerPort, "webhook-server-port", 9443, "Endpoint port for the webhook server.")
-	flag.StringVar(&webhookServiceName, "webhook-service-name", "kubeflow-trainer-controller-manager", "Name of the Service used as part of the DNSName")
-	flag.StringVar(&webhookSecretName, "webhook-secret-name", "kubeflow-trainer-webhook-cert", "Name of the Secret to store CA  and server certs")
+	flag.IntVar(&trainerCfg.CertGeneration.WebhookServerPort, "webhook-server-port", 9443, "Endpoint port for the webhook server.")
+	flag.StringVar(&trainerCfg.CertGeneration.WebhookServiceName, "webhook-service-name", "training-operator-v2", "Name of the Service used as part of the DNSName")
+	flag.StringVar(&trainerCfg.CertGeneration.WebhookSecretName, "webhook-secret-name", "training-operator-v2-webhook-cert", "Name of the Secret to store CA  and server certs")


Please do not change the default values of WebhookServiceName and WebhookSecretName.

Electronic-Waste · 2025-02-25T02:35:39Z

pkg/apis/trainer/v1alpha1/types.go

@@ -0,0 +1,50 @@
+// Copyright 2024 The Kubeflow Authors


Electronic-Waste · 2025-02-25T02:39:06Z

pkg/apis/trainer/v1alpha1/types.go

You can also check the name convention in the Katib. We'd better maintain the consistency between Trainer and Kaitb.

https://github.com/kubeflow/katib/blob/master/pkg/apis/config/v1beta1/types.go

Electronic-Waste · 2025-02-25T02:40:00Z

pkg/apis/trainer/v1alpha1/types.go

+
+	ControllerManager *ControllerManager `json:"controllerManager,omitempty"`
+	CertGeneration   *CertGeneration   `json:"certGeneration,omitempty"`
+}


Try to run make fmt in the terminal, just FYI @chahatsagarmain :)

changes

6ed813d

Signed-off-by: chahatsagarmain <[email protected]>

google-oss-prow bot added the size/M label Feb 9, 2025

google-oss-prow bot requested review from Electronic-Waste and kuizhiqing February 9, 2025 16:08

merge

6393e5a

Signed-off-by: chahatsagarmain <[email protected]>

google-oss-prow bot added size/L and removed size/M labels Feb 9, 2025

andreyvelich reviewed Feb 24, 2025

View reviewed changes

google-oss-prow bot added the ok-to-test label Feb 24, 2025

Electronic-Waste reviewed Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config API for Kubeflow Trainer controller manager #2428

Config API for Kubeflow Trainer controller manager #2428

chahatsagarmain commented Feb 9, 2025

google-oss-prow bot commented Feb 9, 2025

andreyvelich left a comment

andreyvelich Feb 24, 2025

Electronic-Waste Feb 25, 2025

andreyvelich Feb 24, 2025

andreyvelich Feb 24, 2025

Electronic-Waste Feb 25, 2025

andreyvelich Feb 24, 2025

andreyvelich Feb 24, 2025

andreyvelich Feb 24, 2025

andreyvelich Feb 24, 2025

andreyvelich commented Feb 24, 2025

andreyvelich commented Feb 24, 2025

Electronic-Waste left a comment

Electronic-Waste Feb 25, 2025

Electronic-Waste Feb 25, 2025

Electronic-Waste Feb 25, 2025

Electronic-Waste Feb 25, 2025

	type TrainerControllerConfig struct {
	type Configuration struct {

Config API for Kubeflow Trainer controller manager #2428

Are you sure you want to change the base?

Config API for Kubeflow Trainer controller manager #2428

Conversation

chahatsagarmain commented Feb 9, 2025

google-oss-prow bot commented Feb 9, 2025

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Feb 24, 2025

andreyvelich commented Feb 24, 2025

Electronic-Waste left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment