Skip to content
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

[flyte-core] Create separate grpc service for flyteadmin #5168

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion charts/flyte-core/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ helm install gateway bitnami/contour -n flyte
| flyteadmin.resources | object | `{"limits":{"cpu":"250m","ephemeral-storage":"100Mi","memory":"500Mi"},"requests":{"cpu":"10m","ephemeral-storage":"50Mi","memory":"50Mi"}}` | Default resources requests and limits for Flyteadmin deployment |
| flyteadmin.secrets | object | `{}` | |
| flyteadmin.securityContext | object | `{"fsGroup":65534,"fsGroupChangePolicy":"Always","runAsNonRoot":true,"runAsUser":1001,"seLinuxOptions":{"type":"spc_t"}}` | Sets securityContext for flyteadmin pod(s). |
| flyteadmin.service | object | `{"annotations":{"projectcontour.io/upstream-protocol.h2c":"grpc"},"loadBalancerSourceRanges":[],"type":"ClusterIP"}` | Service settings for Flyteadmin |
| flyteadmin.service | object | `{"annotations":{},"grpcAnnotations":{},"httpAnnotations":{},"loadBalancerSourceRanges":[],"type":"ClusterIP"}` | Service settings for Flyteadmin |
| flyteadmin.serviceAccount | object | `{"alwaysCreate":false,"annotations":{},"clusterRole":{"apiGroups":["","flyte.lyft.com","rbac.authorization.k8s.io"],"resources":["configmaps","flyteworkflows","namespaces","pods","resourcequotas","roles","rolebindings","secrets","services","serviceaccounts","spark-role","limitranges"],"verbs":["*"]},"create":true,"createClusterRole":true,"imagePullSecrets":[]}` | Configuration for service accounts for FlyteAdmin |
| flyteadmin.serviceAccount.alwaysCreate | bool | `false` | Should a service account always be created for flyteadmin even without an actual flyteadmin deployment running (e.g. for multi-cluster setups) |
| flyteadmin.serviceAccount.annotations | object | `{}` | Annotations for ServiceAccount attached to Flyteadmin pods |
Expand Down
39 changes: 39 additions & 0 deletions charts/flyte-core/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,45 @@ app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}
{{- end -}}

{{- define "flyteadmin.service.grpc.name" -}}
{{- if .Values.common.ingress.separateGrpcIngress }}
{{- printf "%s-grpc" ( include "flyteadmin.name" . ) -}}
{{- else }}
{{- template "flyteadmin.name" . -}}
{{- end}}
{{- end -}}

{{- define "flyteadmin.service.grpc.port" -}}
{{- if eq .Values.configmap.adminServer.server.security.secure true -}}
80
{{- else -}}
81
{{- end }}
{{- end -}}

{{- define "flyteadmin.service.http.port" -}}
80
Copy link
Contributor

Choose a reason for hiding this comment

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

What if someone needs to override this? I guess putting it into a template would complicate it

{{- end -}}


{{/*
Get the Flyte service GRPC paths.
*/}}
{{- define "flyteadmin.ingress.grpcPaths" -}}
- /flyteidl.service.AdminService
- /flyteidl.service.AdminService/*
- /flyteidl.service.AuthMetadataService
- /flyteidl.service.AuthMetadataService/*
- /flyteidl.service.DataProxyService
- /flyteidl.service.DataProxyService/*
- /flyteidl.service.IdentityService
- /flyteidl.service.IdentityService/*
- /flyteidl.service.SignalService
- /flyteidl.service.SignalService/*
- /grpc.health.v1.Health
- /grpc.health.v1.Health/*
{{- end -}}

{{- define "flytescheduler.name" -}}
flytescheduler
{{- end -}}
Expand Down
9 changes: 6 additions & 3 deletions charts/flyte-core/templates/admin/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,12 @@ spec:
imagePullPolicy: "{{ .Values.flyteadmin.image.pullPolicy }}"
name: flyteadmin
ports:
- containerPort: {{ .Values.configmap.adminServer.server.httpPort }}
- containerPort: {{ .Values.configmap.adminServer.server.grpc.port }}
- containerPort: {{ .Values.configmap.adminServer.flyteadmin.profilerPort }}
- name: http
containerPort: {{ .Values.configmap.adminServer.server.httpPort }}
- name: grpc
containerPort: {{ .Values.configmap.adminServer.server.grpc.port }}
- name: profiler
containerPort: {{ .Values.configmap.adminServer.flyteadmin.profilerPort }}
readinessProbe:
exec:
command: [ "sh", "-c", "reply=$(curl -s -o /dev/null -w %{http_code} http://127.0.0.1:8088/healthcheck); if [ \"$reply\" -lt 200 -o \"$reply\" -ge 400 ]; then exit 1; fi;","grpc_health_probe", "-addr=:8089"]
Expand Down
31 changes: 31 additions & 0 deletions charts/flyte-core/templates/admin/service-grpc.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
{{- if and .Values.flyteadmin.enabled .Values.common.ingress.separateGrpcIngress }}
apiVersion: v1
kind: Service
metadata:
name: {{ include "flyteadmin.service.grpc.name" .}}
namespace: {{ template "flyte.namespace" . }}
labels: {{ include "flyteadmin.labels" . | nindent 4 }}
{{- if or .Values.flyteadmin.service.annotations .Values.flyteadmin.service.grpcAnnotations }}
annotations:
{{- if .Values.flyteadmin.service.annotations }}
{{- tpl ( .Values.flyteadmin.service.annotations | toYaml ) . | nindent 4 }}
{{- end }}
{{- if .Values.flyteadmin.service.grpcAnnotations }}
{{- tpl ( .Values.flyteadmin.service.grpcAnnotations | toYaml ) . | nindent 4 }}
{{- end }}
{{- end }}
spec:
{{- with .Values.flyteadmin.service.type}}
type: {{ . }}
{{- end }}
{{- with .Values.flyteadmin.service.loadBalancerSourceRanges }}
loadBalancerSourceRanges:
{{ . }}
{{- end }}
ports:
- name: grpc
port: {{ include "flyteadmin.service.grpc.port" . }}
protocol: TCP
targetPort: grpc
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this one be 8088 if Values.configmap.adminServer.server.security.secure: true and 8089 otherwise?

selector: {{ include "flyteadmin.selectorLabels" . | nindent 4 }}
{{- end }}
23 changes: 15 additions & 8 deletions charts/flyte-core/templates/admin/service.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,14 @@ metadata:
name: {{ template "flyteadmin.name" . }}
namespace: {{ template "flyte.namespace" . }}
labels: {{ include "flyteadmin.labels" . | nindent 4 }}
{{- with .Values.flyteadmin.service.annotations }}
annotations: {{ tpl (toYaml .) $ | nindent 4 }}
{{- if or .Values.flyteadmin.service.annotations .Values.flyteadmin.service.httpAnnotations }}
annotations:
{{- if .Values.flyteadmin.service.annotations }}
{{- tpl ( .Values.flyteadmin.service.annotations | toYaml ) . | nindent 4 }}
{{- end }}
{{- if .Values.flyteadmin.service.httpAnnotations }}
{{- tpl ( .Values.flyteadmin.service.httpAnnotations | toYaml ) . | nindent 4 }}
{{- end }}
{{- end }}
spec:
{{- with .Values.flyteadmin.service.type}}
Expand All @@ -18,19 +24,20 @@ spec:
{{- end }}
ports:
- name: http
port: 80
port: {{ include "flyteadmin.service.http.port" . }}
protocol: TCP
targetPort: 8088
- name: grpc
port: 81
protocol: TCP
targetPort: 8089
targetPort: http
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I follow, shouldn't this one be 8088 instead of 80?

- name: redoc
protocol: TCP
port: 87
targetPort: 8087
- name: http-metrics
protocol: TCP
port: 10254
{{- if not .Values.common.ingress.separateGrpcIngress }}
- name: grpc
port: {{ include "flyteadmin.service.grpc.port" . }}
targetPort: grpc
{{- end }}
selector: {{ include "flyteadmin.selectorLabels" . | nindent 4 }}
{{- end }}
101 changes: 15 additions & 86 deletions charts/flyte-core/templates/common/ingress.yaml
Original file line number Diff line number Diff line change
@@ -1,95 +1,24 @@
{{- define "grpcRoutes" -}}
{{- $grpcPort := 81 -}}
{{- if eq .Values.configmap.adminServer.server.security.secure true -}}
{{- $grpcPort = 80 -}}
{{- end }}
# NOTE: Port 81 in flyteadmin is the GRPC server port for FlyteAdmin.
- path: /flyteidl.service.SignalService
pathType: ImplementationSpecific
backend:
service:
name: flyteadmin
port:
number: {{ $grpcPort }}
- path: /flyteidl.service.SignalService/*
pathType: ImplementationSpecific
backend:
service:
name: flyteadmin
port:
number: {{ $grpcPort }}
- path: /flyteidl.service.AdminService
pathType: ImplementationSpecific
backend:
service:
name: flyteadmin
port:
number: {{ $grpcPort }}
- path: /flyteidl.service.AdminService/*
pathType: ImplementationSpecific
backend:
service:
name: flyteadmin
port:
number: {{ $grpcPort }}
- path: /flyteidl.service.DataProxyService
pathType: ImplementationSpecific
backend:
service:
name: flyteadmin
port:
number: {{ $grpcPort }}
- path: /flyteidl.service.DataProxyService/*
pathType: ImplementationSpecific
backend:
service:
name: flyteadmin
port:
number: {{ $grpcPort }}
- path: /flyteidl.service.AuthMetadataService
pathType: ImplementationSpecific
backend:
service:
name: flyteadmin
port:
number: {{ $grpcPort }}
- path: /flyteidl.service.AuthMetadataService/*
pathType: ImplementationSpecific
backend:
service:
name: flyteadmin
port:
number: {{ $grpcPort }}
- path: /flyteidl.service.IdentityService
pathType: ImplementationSpecific
backend:
service:
name: flyteadmin
port:
number: {{ $grpcPort }}
- path: /flyteidl.service.IdentityService/*
pathType: ImplementationSpecific
backend:
service:
name: flyteadmin
port:
number: {{ $grpcPort }}
- path: /grpc.health.v1.Health
pathType: ImplementationSpecific
backend:
service:
name: flyteadmin
port:
number: {{ $grpcPort }}
- path: /grpc.health.v1.Health/*
{{- $paths := (include "flyteadmin.ingress.grpcPaths" .) | fromYamlArray }}
{{- range $path := $paths }}
- path: {{ $path }}
{{- if semverCompare ">=1.18-0" $.Capabilities.KubeVersion.GitVersion }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure Capabilities.KubeVersion.GitVersion is still supported on Helm 3 (ref)

pathType: ImplementationSpecific
{{- end }}
backend:
{{- if semverCompare ">=1.19-0" $.Capabilities.KubeVersion.GitVersion }}
service:
name: flyteadmin
name: {{ include "flyteadmin.service.grpc.name" $ }}
port:
number: {{ $grpcPort }}
{{- end }}
{{- if .Values.common.ingress.enabled }}
number: {{ include "flyteadmin.service.grpc.port" $ }}
{{- else }}
serviceName: {{ include "flyteadmin.service.grpc.name" $ }}
servicePort: {{ include "flyteadmin.service.grpc.port" $ }}
{{- end }}
{{- end }}
{{- end }}
{{- if .Values.common.ingress.enabled }}
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
Expand Down
8 changes: 4 additions & 4 deletions charts/flyte-core/values-gcp.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ userSettings:
dbHost: <CLOUD-SQL-IP>
dbPassword: <DBPASSWORD>
# These two storage buckets could be the same or you could specify different buckets if required. Both keys are required.
# Learn more https://docs.flyte.org/en/latest/concepts/data_management.html#understand-how-flyte-handles-data
bucketName: <METADATA_BUCKET_NAME>
rawDataBucketName: <RAW_DATA_BUCKET_NAME>
# Learn more https://docs.flyte.org/en/latest/concepts/data_management.html#understand-how-flyte-handles-data
bucketName: <METADATA_BUCKET_NAME>
rawDataBucketName: <RAW_DATA_BUCKET_NAME>
hostName: <HOSTNAME>

#
Expand All @@ -35,7 +35,7 @@ flyteadmin:
ephemeral-storage: 2Gi
memory: 1G
service:
annotations:
grpcAnnotations:
# Required for the ingress to properly route grpc traffic to grpc port
cloud.google.com/app-protocols: '{"grpc":"HTTP2"}'
affinity:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ flyteadmin:
- flyteexamples
# -- Service settings for Flyteadmin
service:
annotations:
grpcAnnotations:
projectcontour.io/upstream-protocol.h2c: grpc
type: ClusterIP
loadBalancerSourceRanges: []
Expand Down
2 changes: 1 addition & 1 deletion charts/flyte-core/values-sandbox.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ flyteadmin:
serviceMonitor:
enabled: false
service:
annotations:
grpcAnnotations:
projectcontour.io/upstream-protocol.h2c: grpc
type: ClusterIP
loadBalancerSourceRanges: []
Expand Down
7 changes: 5 additions & 2 deletions charts/flyte-core/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,11 @@ flyteadmin:
- flyteexamples
# -- Service settings for Flyteadmin
service:
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see the Values.common.ingress.separateGrpcIngress key added to values

annotations:
projectcontour.io/upstream-protocol.h2c: grpc
annotations: {}
httpAnnotations: {}
grpcAnnotations: {}
# projectcontour.io/upstream-protocol.h2c: grpc
# traefik.ingress.kubernetes.io/service.serversscheme: h2c
type: ClusterIP
loadBalancerSourceRanges: []
# -- Configuration for service accounts for FlyteAdmin
Expand Down
Loading
Loading