Skip to content

[Feature] Add cleanup for terminated RayJob/RayCluster metrics #3923

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Aug 29, 2025
Merged

[Feature] Add cleanup for terminated RayJob/RayCluster metrics #3923

merged 13 commits into from
Aug 29, 2025

Conversation

phantom5125
Copy link
Contributor

@phantom5125 phantom5125 commented Aug 7, 2025 •
edited
Loading

Why are these changes needed?

Since some of our metrics are permanently stored in Prometheus, that might cause the /metrics endpoint to become slow or time out, we need a lifecycle-based cleanup.

Related issue number

Closes #3820

End-to-end test example

$kubectl apply -f ray-operator/config/samples/ray-job.sample.yaml

# $kubectl port-forward <kuberay-operator-pod-name> 8080:8080

$curl -s 127.0.0.1:8080/metrics | grep kuberay_
# HELP kuberay_cluster_condition_provisioned Indicates whether the RayCluster is provisioned
# TYPE kuberay_cluster_condition_provisioned gauge
kuberay_cluster_condition_provisioned{condition="true",name="rayjob-sample-clwvk",namespace="default"} 1
# HELP kuberay_cluster_info Metadata information about RayCluster custom resources
# TYPE kuberay_cluster_info gauge
kuberay_cluster_info{name="rayjob-sample-clwvk",namespace="default",owner_kind="RayJob"} 1
# HELP kuberay_cluster_provisioned_duration_seconds The time, in seconds, when a RayCluster's `RayClusterProvisioned` status transitions from false (or unset) to true
# TYPE kuberay_cluster_provisioned_duration_seconds gauge
kuberay_cluster_provisioned_duration_seconds{name="rayjob-sample-clwvk",namespace="default"} 1259.406597953
...

After we clean CR, there will be no more metrics

$kubectl delete rayjob rayjob-sample
$curl -s 127.0.0.1:8080/metrics | grep kuberay_

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@phantom5125 phantom5125 marked this pull request as draft August 7, 2025 19:02
@troychiu
Copy link
Contributor

troychiu commented Aug 7, 2025

Hi @phantom5125 , thank you for creating the PR. Just want to make sure we are in the same page before you start polishing the PR. Do we really need the TTL-based cleanup? I was thinking cleaning up the metric as long as the CR is deleted.

@phantom5125
Copy link
Contributor Author

Hi @phantom5125 , thank you for creating the PR. Just want to make sure we are in the same page before you start polishing the PR. Do we really need the TTL-based cleanup? I was thinking cleaning up the metric as long as the CR is deleted.

Thanks for your notice!

From my perspective, the independent metricsTTL is primarily intended to address the scenario where JobTTLSeconds is set to 0. In this case, the RayJob CR is deleted immediately after the job finishes. Then metrics like kuberay_job_execution_duration_seconds may not be collected, because it will likely to be deleted as soon as the metric is produced.

@troychiu
Copy link
Contributor

troychiu commented Aug 9, 2025 •
edited
Loading

Hi @phantom5125 , thank you for creating the PR. Just want to make sure we are in the same page before you start polishing the PR. Do we really need the TTL-based cleanup? I was thinking cleaning up the metric as long as the CR is deleted.

Thanks for your notice!

From my perspective, the independent metricsTTL is primarily intended to address the scenario where JobTTLSeconds is set to 0. In this case, the RayJob CR is deleted immediately after the job finishes. Then metrics like kuberay_job_execution_duration_seconds may not be collected, because it will likely to be deleted as soon as the metric is produced.

I think introducing TTL-based cleanup is overkill for this scenario. Instead, we can simply document that setting JobTTLSeconds to a value smaller than the Prometheus scrape interval may cause metrics to be deleted before Prometheus can collect them. I just think we can start with a simpler implementation. What do you think?

@phantom5125
Copy link
Contributor Author

I think introducing TTL-based cleanup is overkill for this scenario. Instead, we can simply document that setting JobTTLSeconds to a value smaller than the Prometheus scrape interval may cause metrics to be deleted before Prometheus can collect them. I just think we can start with a simpler implementation. What do you think?

Ok, I will take your suggestion and update the PR soon!

@phantom5125 phantom5125 changed the title [Feature] Add TTL-based cleanup for terminated RayJob/RayCluster metrics [Feature] Add cleanup for terminated RayJob/RayCluster metrics Aug 9, 2025
@phantom5125 phantom5125 marked this pull request as ready for review August 9, 2025 19:56
@phantom5125
Copy link
Contributor Author

@troychiu PTAL, thanks!

troychiu
troychiu reviewed Aug 12, 2025
View reviewed changes
Copy link
Contributor

@troychiu troychiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the contribution!

@phantom5125 phantom5125 requested a review from troychiu August 13, 2025 16:17
win5923
win5923 reviewed Aug 13, 2025
View reviewed changes
troychiu
troychiu reviewed Aug 13, 2025
View reviewed changes
@phantom5125 phantom5125 requested a review from troychiu August 14, 2025 17:20
troychiu
troychiu approved these changes Aug 20, 2025
View reviewed changes
Copy link
Contributor

@troychiu troychiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thank you! cc @kevin85421

@kevin85421
Copy link
Member

cc @rueian could you review this PR? Thanks!

@phantom5125
Copy link
Contributor Author

@rueian PTAL, thanks!

rueian
rueian reviewed Aug 25, 2025
View reviewed changes
ray-operator/controllers/ray/metrics/ray_cluster_metrics.go Outdated
// NOTE: Uses Delete() as metric has only "name" and "namespace" labels.
// If more labels are added, switch to DeletePartialMatch(), otherwise it may not clean up all metrics correctly.
func (r *RayClusterMetricsManager) DeleteRayClusterMetrics(name, namespace string) {
numCleanedUpMetrics := r.rayClusterProvisionedDurationSeconds.Delete(prometheus.Labels{"name": name, "namespace": namespace})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @phantom5125, I think we should use your original proposal, DeletePartialMatch, now. Having a comment here doesn't help us switch to DeletePartialMatch in the future, I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rueian Thank you for the review! Fixed in 40e1831

@phantom5125 phantom5125 requested a review from rueian August 28, 2025 11:29
@phantom5125
Copy link
Contributor Author

This PR has passed CI and received approval, I believe this is ready to merge.
@kevin85421 could you please take a look? Thanks!

@rueian rueian merged commit d56356b into ray-project:master Aug 29, 2025
12 checks passed
@rueian
Copy link
Contributor

rueian commented Aug 29, 2025

Thank you @phantom5125

@troychiu troychiu mentioned this pull request Aug 31, 2025
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rueian rueian rueian approved these changes

@kevin85421 kevin85421 Awaiting requested review from kevin85421 kevin85421 is a code owner

@andrewsykim andrewsykim Awaiting requested review from andrewsykim andrewsykim is a code owner

@MortalHappiness MortalHappiness Awaiting requested review from MortalHappiness MortalHappiness is a code owner

+2 more reviewers

@win5923 win5923 win5923 left review comments

@troychiu troychiu troychiu approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@troychiu troychiu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Add prometheus metrics reset support
5 participants
@phantom5125 @troychiu @kevin85421 @rueian @win5923