[flyte-core] Create separate grpc service for flyteadmin #5168
Conversation
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
dosubot
bot
added
size:M
Jeinhaus
force-pushed
the
feature/add-separate-grpc-service-for-flyte-core
branch
from
2f17c21 to
44eaba8
Compare
I think |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5168 +/- ##
===========================================
- Coverage 76.69% 59.95% -16.75%
===========================================
Files 18 463 +445
Lines 1515 37490 +35975
===========================================
+ Hits 1162 22477 +21315
- Misses 289 13329 +13040
- Partials 64 1684 +1620
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Sounds good. I'll give it a shot. |
Jeinhaus
force-pushed
the
feature/add-separate-grpc-service-for-flyte-core
branch
from
efd9226 to
8fd7240
Compare
dosubot
bot
added
size:XXL
This allows setting annotations that are required for some ingress controllers for grpc communication only on the parts that actually use grpc. Without this separation either the console or the grpc endpoints did not work properly with some ingress controllers, e.g. traefik. Signed-off-by: Julian Einhaus <[email protected]>
Jeinhaus
force-pushed
the
feature/add-separate-grpc-service-for-flyte-core
branch
from
8fd7240 to
965a977
Compare
dosubot
bot
added
size:XL
Signed-off-by: Julian Einhaus <[email protected]>
Jeinhaus
force-pushed
the
feature/add-separate-grpc-service-for-flyte-core
branch
from
965a977 to
276679c
Compare
This got quite big, because I tried to make it kinda similar to the |
| - name: grpc | ||
| port: {{ include "flyteadmin.service.grpc.port" . }} | ||
| protocol: TCP | ||
| targetPort: grpc |
There was a problem hiding this comment.
Shouldn't this one be 8088 if Values.configmap.adminServer.server.security.secure: true and 8089 otherwise?
| {{- end -}} | ||
|
|
||
| {{- define "flyteadmin.service.http.port" -}} | ||
| 80 |
There was a problem hiding this comment.
What if someone needs to override this? I guess putting it into a template would complicate it
| port: 81 | ||
| protocol: TCP | ||
| targetPort: 8089 | ||
| targetPort: http |
There was a problem hiding this comment.
Not sure I follow, shouldn't this one be 8088 instead of 80?
| {{- $paths := (include "flyteadmin.ingress.grpcPaths" .) | fromYamlArray }} | ||
| {{- range $path := $paths }} | ||
| - path: {{ $path }} | ||
| {{- if semverCompare ">=1.18-0" $.Capabilities.KubeVersion.GitVersion }} |
There was a problem hiding this comment.
I'm not quite sure Capabilities.KubeVersion.GitVersion is still supported on Helm 3 (ref)
| @@ -50,8 +50,11 @@ flyteadmin: | |||
| - flyteexamples | |||
| # -- Service settings for Flyteadmin | |||
| service: | |||
There was a problem hiding this comment.
I didn't see the Values.common.ingress.separateGrpcIngress key added to values
|
@Jeinhaus any chance you can take a look at the comments? Thanks for putting so much effort into this! |
|
This seems quite far and would be great to have! @Jeinhaus any chance you get to work on this still? |
Tracking issue
#4962
Why are the changes needed?
This allows setting annotations that are required for some ingress controllers for grpc communication only on parts that actually use grpc.
Without this separation, either the console or the grpc endpoints did not work properly with some ingress controllers, e.g. traefik.
What changes were proposed in this pull request?
This PR splits the existing
flyteadminservice of theflyte-corechart into two separate services (like in theflyte-binarychart).One service for grpc communication and another one for all other communication.
Accordingly, the
annotationsfield of thevalues.yamlwas extended to also usehttpAnnotationsandgrpcAnnotations.Technically, this is a breaking change for contour users, because the
values.yamlcontained a defaultannotation(that I think should not really be there anyway):I removed this default, but we could also keep this for backwards compatibility.
Docs link
I'm not sure how to regenerate the docs for this chart from the
values.yaml. Is there some command I can run?