-
Notifications
You must be signed in to change notification settings - Fork 168
epp servicemonitor #1425
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
epp servicemonitor #1425
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
{{- if or .Values.inferenceExtension.monitoring.prometheus.enabled .Values.inferenceExtension.monitoring.gke.enabled }} | ||
apiVersion: v1 | ||
kind: Secret | ||
metadata: | ||
name: {{ .Values.inferenceExtension.monitoring.secret.name }} | ||
namespace: {{ .Release.Namespace }} | ||
labels: | ||
{{- include "gateway-api-inference-extension.labels" . | nindent 4 }} | ||
annotations: | ||
kubernetes.io/service-account.name: {{ include "gateway-api-inference-extension.name" . }} | ||
type: kubernetes.io/service-account-token | ||
{{- end }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
{{- if .Values.inferenceExtension.monitoring.prometheus.enabled }} | ||
apiVersion: monitoring.coreos.com/v1 | ||
kind: ServiceMonitor | ||
metadata: | ||
name: {{ include "gateway-api-inference-extension.name" . }}-monitor | ||
namespace: {{ .Release.Namespace }} | ||
labels: | ||
{{- include "gateway-api-inference-extension.labels" . | nindent 4 }} | ||
spec: | ||
endpoints: | ||
- interval: {{ .Values.inferenceExtension.monitoring.interval }} | ||
port: "http-metrics" | ||
path: "/metrics" | ||
authorization: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If ServiceMonitor is namespace-scoped, does the secret need to reside in the same namespace of the CR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK - but I've never run with the secret in another ns so not 100% sure - definitely I'd say best practice though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (namespace is included in the secret template) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT there is no option to add namespace in that authorization.credentials section. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see that both the CRD and secret use the namespace template |
||
credentials: | ||
key: token | ||
name: {{ .Values.inferenceExtension.monitoring.secret.name }} | ||
jobLabel: {{ include "gateway-api-inference-extension.name" . }} | ||
namespaceSelector: | ||
matchNames: | ||
- {{ .Release.Namespace }} | ||
selector: | ||
matchLabels: | ||
{{- include "gateway-api-inference-extension.labels" . | nindent 6 }} | ||
{{- end }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,15 +46,15 @@ spec: | |
endpoints: | ||
- port: metrics | ||
scheme: http | ||
interval: 5s | ||
interval: {{ .Values.inferenceExtension.monitoring.interval }} | ||
path: /metrics | ||
authorization: | ||
type: Bearer | ||
credentials: | ||
secret: | ||
name: {{ .Values.gke.monitoringSecret.name }} | ||
name: {{ .Values.inferenceExtension.monitoring.secret.name }} | ||
key: token | ||
namespace: {{ .Values.gke.monitoringSecret.namespace }} | ||
namespace: {{ .Release.Namespace }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JeffLuoo this means we need to change the GKE docs since we now have to create a secret under the pool's namespace and we need to create one for each epp deployment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The update in this PR includes a new file |
||
selector: | ||
matchLabels: | ||
{{- include "gateway-api-inference-extension.selectorLabels" . | nindent 8 }} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,17 @@ inferenceExtension: | |
|
||
tolerations: [] | ||
|
||
# Monitoring configuration for EPP | ||
monitoring: | ||
interval: "10s" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Make the default scraping interval See https://grafana.com/blog/2020/09/28/new-in-grafana-7.2-__rate_interval-for-prometheus-rate-queries-that-just-work that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I came across this, which is why I set to 10s - says to set to less than 15s - wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah thanks for catching this. I checked the dashboard and we are all using |
||
# Service account token secret for authentication | ||
secret: | ||
name: inference-gateway-sa-metrics-reader-secret | ||
|
||
# Prometheus ServiceMonitor will be created when enabled for EPP metrics collection | ||
prometheus: | ||
enabled: false | ||
|
||
inferencePool: | ||
targetPorts: | ||
- number: 8000 | ||
|
@@ -56,7 +67,3 @@ inferencePool: | |
provider: | ||
name: none | ||
|
||
gke: | ||
monitoringSecret: | ||
name: inference-gateway-sa-metrics-reader-secret | ||
namespace: default |
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.
same comment, add namespace (I assume this is namespace scoped?)
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