Config API for Kubeflow Trainer controller manager #2428
Conversation
Signed-off-by: chahatsagarmain <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: chahatsagarmain <[email protected]>
There was a problem hiding this comment.
Thank you for this @chahatsagarmain!
/assign @Electronic-Waste @kubeflow/wg-training-leads @astefanutti Please can you also take a look ?
| @@ -0,0 +1,50 @@ | |||
| // Copyright 2024 The Kubeflow Authors | |||
There was a problem hiding this comment.
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
| ) | ||
|
|
||
| // TrainerControllerConfig represents the configuration for the training operator. | ||
| type TrainerControllerConfig struct { |
There was a problem hiding this comment.
Let's call it type Configuration struct
| type TrainerControllerConfig struct { | |
| type Configuration struct { |
|
|
||
| ControllerManager *ControllerManager `json:"controllerManager,omitempty"` | ||
| CertGeneration *CertGeneration `json:"certGeneration,omitempty"` | ||
| } |
There was a problem hiding this comment.
You might need to fix Go formatting here.
| WebhookServerPort int `json:"webhookServerPort,omitempty"` | ||
| WebhookServiceName string `json:"webhookServiceName,omitempty"` | ||
| WebhookSecretName string `json:"webhookSecretName,omitempty"` |
There was a problem hiding this comment.
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
| } | ||
|
|
||
| // CertGeneration holds configuration related to webhook server certificate generation. | ||
| type CertGeneration struct { |
There was a problem hiding this comment.
Should we also have flag to enable/disable Cert Generator ?
| 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"` | ||
| } |
There was a problem hiding this comment.
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.
| "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.") |
There was a problem hiding this comment.
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
|
/ok-to-test |
|
Here is an example on how to validate and read config: https://github.com/kubernetes-sigs/jobset/tree/main/pkg/config |
What this PR does / why we need it:
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: