-
Notifications
You must be signed in to change notification settings - Fork 168
add gke monitoring helm support. #1600
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
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 |
---|---|---|
|
@@ -35,11 +35,44 @@ spec: | |
timeoutSec: 300 # 5-minute timeout (adjust as needed) | ||
logging: | ||
enabled: true # log all requests by default | ||
{{- if .Values.inferenceExtension.monitoring.gke.enabled }} | ||
{{- $metricsReadSA := printf "%s-metrics-reader-sa" .Release.Name -}} | ||
{{- $metricsReadSecretName := printf "%s-metrics-reader-secret" .Release.Name -}} | ||
{{- $metricsReadRoleName := printf "%s-%s-metrics-reader" .Release.Namespace .Release.Name -}} | ||
{{- $metricsReadRoleBindingName := printf "%s-%s-metrics-reader-role-binding" .Release.Namespace .Release.Name -}} | ||
{{- $secretReadRoleName := printf "%s-metrics-reader-secret-read" .Release.Name -}} | ||
{{- $gmpNamespace := "gmp-system" -}} | ||
{{- $isAutopilot := false -}} | ||
{{- with .Values.provider.gke }} | ||
{{- $isAutopilot = .autopilot | default false -}} | ||
{{- end }} | ||
{{- if $isAutopilot -}} | ||
{{- $gmpNamespace = "gke-gmp-system" -}} | ||
{{- end -}} | ||
{{- $gmpCollectorRoleBindingName := printf "%s:collector:%s-%s-metrics-reader-secret-read" $gmpNamespace .Release.Namespace .Release.Name -}} | ||
--- | ||
apiVersion: v1 | ||
kind: ServiceAccount | ||
metadata: | ||
name: {{ $metricsReadSA }} | ||
namespace: {{ .Release.Namespace }} | ||
--- | ||
apiVersion: v1 | ||
kind: Secret | ||
metadata: | ||
name: {{ $metricsReadSecretName }} | ||
namespace: {{ .Release.Namespace }} | ||
labels: | ||
{{- include "gateway-api-inference-extension.labels" . | nindent 4 }} | ||
annotations: | ||
kubernetes.io/service-account.name: {{ $metricsReadSA }} | ||
type: kubernetes.io/service-account-token | ||
--- | ||
apiVersion: monitoring.googleapis.com/v1 | ||
kind: ClusterPodMonitoring | ||
kind: PodMonitoring | ||
metadata: | ||
name: {{ .Release.Namespace }}-{{ .Release.Name }} | ||
name: {{ .Release.Name }} | ||
namespace: {{ .Release.Namespace }} | ||
labels: | ||
{{- include "gateway-api-inference-extension.labels" . | nindent 4 }} | ||
spec: | ||
|
@@ -52,10 +85,58 @@ spec: | |
type: Bearer | ||
credentials: | ||
secret: | ||
name: {{ .Values.inferenceExtension.monitoring.secret.name }} | ||
name: {{ $metricsReadSecretName }} | ||
key: token | ||
namespace: {{ .Release.Namespace }} | ||
selector: | ||
matchLabels: | ||
{{- include "gateway-api-inference-extension.selectorLabels" . | nindent 8 }} | ||
--- | ||
apiVersion: rbac.authorization.k8s.io/v1 | ||
kind: ClusterRole | ||
metadata: | ||
name: {{ $metricsReadRoleName }} | ||
rules: | ||
- nonResourceURLs: | ||
- /metrics | ||
verbs: | ||
- get | ||
--- | ||
apiVersion: rbac.authorization.k8s.io/v1 | ||
kind: ClusterRoleBinding | ||
metadata: | ||
name: {{ $metricsReadRoleBindingName }} | ||
subjects: | ||
- kind: ServiceAccount | ||
name: {{ $metricsReadSA }} | ||
namespace: {{ .Release.Namespace }} | ||
roleRef: | ||
kind: ClusterRole | ||
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. Let's not make new ClusterRoles, #1393 is asking converting existing Cluster RBAC to namespace scoped. 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 we use a namespaced scope rbac, make sure all namespace align: epp, secret, and the namespace in cluster pod monitoring. Also, since we are using a namespace scoped objects, we can consider using the podMonitoring instead a cluster pod monitoring, where the podMonitoring is also namespace scoped. 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 does PodMonitoring exist outside of GKE? 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.
@sallyom adds an option using 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. or did you mean PodMonitor from Prometheus operator? 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 made a new commit(2d9a5e5) to make most of the Cluster scoped resource to namespaced. However, the following two resources is kept to make the gmp scraping metrics
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. Yes. 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. which two roles are you referring to? the current commit is a working version. just having the ClusterRole and ClusterRoleBinding for metrics read. 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 attempted a summary of this in #1393 (comment). TLDR is that if Cluster RBAC is unavoidable, we can use uniquely named Cluster RBAC names to avoid collision. 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. SG, thanks, amend the commit, now I only leave the ClusterRole and ClusterRoleBinding for GKE metrics read. And changed the name to include the namespace. Updated the PR description to reflect what we have now in the helm. |
||
name: {{ $metricsReadRoleName }} | ||
apiGroup: rbac.authorization.k8s.io | ||
--- | ||
apiVersion: rbac.authorization.k8s.io/v1 | ||
kind: Role | ||
metadata: | ||
name: {{ $secretReadRoleName }} | ||
rules: | ||
- resources: | ||
- secrets | ||
apiGroups: [""] | ||
verbs: ["get", "list", "watch"] | ||
resourceNames: [{{ $metricsReadSecretName | quote }}] | ||
--- | ||
apiVersion: rbac.authorization.k8s.io/v1 | ||
kind: RoleBinding | ||
metadata: | ||
name: {{ $gmpCollectorRoleBindingName }} | ||
namespace: {{ .Release.Namespace }} | ||
roleRef: | ||
name: {{ $secretReadRoleName }} | ||
kind: Role | ||
apiGroup: rbac.authorization.k8s.io | ||
subjects: | ||
- name: collector | ||
namespace: {{ $gmpNamespace }} | ||
kind: ServiceAccount | ||
{{- end }} | ||
{{- end }} |
Uh oh!
There was an error while loading. Please reload this page.