-
Notifications
You must be signed in to change notification settings - Fork 330
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.
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
Customize the Prometheus export port #954
Customize the Prometheus export port #954
Conversation
@@ -137,8 +137,6 @@ func DefaultHeadPodTemplate(instance rayiov1alpha1.RayCluster, headSpec rayiov1a | |||
} | |||
if dupIndex < 0 { | |||
podTemplate.Spec.Containers[0].Ports = append(podTemplate.Spec.Containers[0].Ports, metricsPort) | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i am reading this right, if user does not provide metrics port then we wont provide a default anymore.
Curious why we cannot have both?
- By default exposes Prometheus on port 8080
- Allow users to override
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! we will have the default Prometheus port 8080.
// add metrics port for exposing to the promethues stack.
metricsPort := v1.ContainerPort{
Name: "metrics",
ContainerPort: int32(DefaultMetricsPort),
}
dupIndex := -1
for i, port := range podTemplate.Spec.Containers[0].Ports {
if port.Name == metricsPort.Name {
dupIndex = i
break
}
}
// dupIndex < 0 means the user does not set metricsPort, so we set the default metricsPort.
if dupIndex < 0 {
podTemplate.Spec.Containers[0].Ports = append(podTemplate.Spec.Containers[0].Ports, metricsPort)
}
// If the user does set metricsPort, we will now not override with the default metricsPort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me, but I do not see any tests. Are you still working on the tests or are the tests covered somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added tests. Thank you!
Signed-off-by: Yicheng-Lu-llll <luyc58576@gmail.com>
Signed-off-by: Yicheng-Lu-llll <luyc58576@gmail.com>
Signed-off-by: Yicheng-Lu-llll <luyc58576@gmail.com>
a91e140
to
d005170
Compare
Signed-off-by: Yicheng-Lu-llll <luyc58576@gmail.com>
Signed-off-by: Yicheng-Lu-llll <luyc58576@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Customize the Prometheus export port
Why are these changes needed?
Currently, KubeRay always exposes a Prometheus metrics endpoint on port 8080, and users are unable to customize this export port (see issue #864). However, the Ray documentation indicates that users are able to set up a custom Prometheus export port.
To better meet the needs of users and maintain consistency with Ray's capabilities, I recommend enabling the customization of the Prometheus export port for KubeRay users.
How to set Prometheus export port ?
metrics-export-port: "{Port Number}"
torayStartParams
for head and worker pods.containerPort: {Port Number}
totemplate.spec.containers[0].ports
for head and worker pods.Note, It's user's responsibility to maintain rayStartParam ports and container ports mapping(see here)
For example:
(full version can be found here)
Related issue number
Closes #864
Checks
ray-cluster.custom-prometheus-export-port.yaml
set metrics endpoint in port 9091(Can be found here)