[abhijeet-dhumal]

What this PR does

Adds optional Prometheus monitoring configuration for the Trainer controller.

• Adds a Kustomize monitoring overlay with a Prometheus Operator ServiceMonitor.
• Adds Helm chart support for enabling the ServiceMonitor.
• Selects the controller Service using stable labels and the monitoring-port.
• Configures HTTP or HTTPS based on metrics secure-serving settings.
• Adds optional bearer-token configuration when metrics authentication is enabled.
• Supports overriding the ServiceMonitor CA Secret, key, and TLS server name.
• Reuses the existing webhook certificate by default.

Scope

This PR addresses the scrapeability portion of #3429 for the existing controller-runtime metrics endpoint. It does not add Trainer-specific metrics or runtime workload instrumentation.

The monitoring resources are opt-in and require the Prometheus Operator CRDs. Prometheus RBAC and platform-specific authentication configuration remain deployment-specific. Authentication over HTTP is rejected by the Helm chart.

Validation

• Helm unit tests covering disabled, secure, authenticated, insecure, invalid auth/TLS, and custom TLS configurations.
• Helm lint.
• Kustomize build for the monitoring overlay.
• The ServiceMonitor selects the controller Service and uses the existing metrics certificate by default.
• Real cluster validation:
  • Applied the rendered Helm ServiceMonitor successfully.
  • Confirmed Prometheus discovered the Trainer target.
  • Confirmed the Trainer HTTPS metrics endpoint returned HTTP 200 and exposed controller metrics.
  • Full healthy scrape validation requires deploying the current Trainer image containing the latest secure metrics certificate handling.

Follow-up

• Define and implement Trainer-specific metrics and naming conventions.
• Integrate the monitoring configuration into supported platform installation paths as needed.

[google-oss-prow]

@ugiordan: changing LGTM is restricted to collaborators

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[robert-bell]

Thanks @abhijeet-dhumal!

Could you also add the service monitor to the helm chart please?

[abhijeet-dhumal]

Thanks @abhijeet-dhumal!

Could you also add the service monitor to the helm chart please?

Thanks @robert-bell !
Added an opt-in Helm ServiceMonitor via metrics.serviceMonitor.enabled, including HTTPS, the existing CA Secret, and the Service DNS name.

[robert-bell]

Thanks @abhijeet-dhumal!

/lgtm

cc @andreyvelich ready for approval

[abhijeet-dhumal]

Thanks @abhijeet-dhumal!

/lgtm

cc @andreyvelich ready for approval

@andreyvelich Are we good to go with this PR?

[Sridhar1030]

/lgtm as well

[shreyaabaranwal]

Thanks for this, @abhijeet-dhumal. Since the chart now exposes metrics.auth.enabled,
one thing I noticed while reading: when auth is enabled, the metrics filter calls
TokenReview and SubjectAccessReview (pkg/metrics/setup.go, lines 57-58). The
controller ClusterRole in manifests/base/rbac/role.yaml grants create on both
(lines 88, 94), but the Helm ClusterRole (charts/kubeflow-trainer/templates/rbac/
clusterrole.yaml) doesn't, and this PR doesn't change that file.

I've only checked this by reading the code and rendering the templates, not on a
live cluster. Should those two rules be added to the Helm ClusterRole in this PR,
or would a follow-up be better? Happy to take the follow-up if you'd prefer.

[robert-bell]

Since the chart now exposes metrics.auth.enabled,
one thing I noticed while reading: when auth is enabled, the metrics filter calls
TokenReview and SubjectAccessReview (pkg/metrics/setup.go, lines 57-58). The
controller ClusterRole in manifests/base/rbac/role.yaml grants create on both
(lines 88, 94), but the Helm ClusterRole (charts/kubeflow-trainer/templates/rbac/
clusterrole.yaml) doesn't, and this PR doesn't change that file.

Thanks for spotting this @shreyaabaranwal - I think you're right this is missing from the helm chart.

@abhijeet-dhumal could you add subjectaccessreviews and tokenreviews into the helm chart rbac in this PR?

[abhijeet-dhumal]

Thanks for this, @abhijeet-dhumal. Since the chart now exposes metrics.auth.enabled, one thing I noticed while reading: when auth is enabled, the metrics filter calls TokenReview and SubjectAccessReview (pkg/metrics/setup.go, lines 57-58). The controller ClusterRole in manifests/base/rbac/role.yaml grants create on both (lines 88, 94), but the Helm ClusterRole (charts/kubeflow-trainer/templates/rbac/ clusterrole.yaml) doesn't, and this PR doesn't change that file.

I've only checked this by reading the code and rendering the templates, not on a live cluster. Should those two rules be added to the Helm ClusterRole in this PR, or would a follow-up be better? Happy to take the follow-up if you'd prefer.

Thanks @shreyaabaranwal for catching this. @robert-bell I have added the required TokenReview and SubjectAccessReview permissions to the Helm ClusterRole, matching the base manifests, and added unit-test coverage.

[robert-bell]

Aside from Andrey's small comment this looks great - thanks @abhijeet-dhumal!

/lgtm

[abhijeet-dhumal]

Thanks @andreyvelich and @robert-bell. All remaining feedback is addressed, including renaming the overlay to manifests/overlays/prometheus. Kustomize validation passes and no stale references remain.
Please review and add LGTM if it looks good. 🙌

[abhijeet-dhumal]

Rebasing on #4217 to resolve ee test PR checks failures

[robert-bell]

/lgtm

Thanks @abhijeet-dhumal!

[andreyvelich]

Thanks for this work @abhijeet-dhumal!
/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [andreyvelich]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[andreyvelich]

Quick question here, @abhijeet-dhumal @robert-bell.
Do we want to use prometheus or metrics in the Helm Chart values to be consistent with JobSet and Kueue?

• https://github.com/kubernetes-sigs/jobset/blob/main/charts/jobset/values.yaml#L108
• https://github.com/kubernetes-sigs/kueue/blob/main/charts/kueue/values.yaml#L388

[robert-bell]

Yeah we should be consistent. I'd lean towards prometheus which is also what cert-manager uses.

[andreyvelich]

Sounds good to me, @abhijeet-dhumal please could you create PR to update it to prometheus?

[robert-bell]

@andreyvelich - #4244