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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 2 commits into from

Conversation

Jeinhaus
Copy link
Contributor

@Jeinhaus Jeinhaus commented Apr 3, 2024 •
edited

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 flyteadmin service of the flyte-core chart into two separate services (like in the flyte-binary chart).
One service for grpc communication and another one for all other communication.
Accordingly, the annotations field of the values.yaml was extended to also use httpAnnotations and grpcAnnotations.
Technically, this is a breaking change for contour users, because the values.yaml contained a default annotation (that I think should not really be there anyway):

    annotations:
      projectcontour.io/upstream-protocol.h2c: grpc

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?

@welcome Welcome
Copy link

welcome bot commented Apr 3, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Apr 3, 2024
@Jeinhaus Jeinhaus force-pushed the feature/add-separate-grpc-service-for-flyte-core branch from 2f17c21 to 44eaba8 Compare April 3, 2024 08:54
davidmirror-ops
davidmirror-ops reviewed Apr 3, 2024
View reviewed changes
Copy link
Contributor

@davidmirror-ops davidmirror-ops left a comment

Choose a reason for hiding this comment

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

I think we should make this optional (just as in flyte-binary) and probably leave the default behavior to be single service. wdyt?

@davidmirror-ops
Copy link
Contributor

I'm not sure how to regenerate the docs for this chart from the values.yaml. Is there some command I can run?

I think make helm should do the trick

@codecov Codecov
Copy link

codecov bot commented Apr 5, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.95%. Comparing base (6d58c73) to head (44eaba8).
Report is 1 commits behind head on master.

❗ Current head 44eaba8 differs from pull request most recent head 276679c. Consider uploading reports for the commit 276679c to get more accurate results

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
Flag Coverage Δ
unittests 59.95% <ø> (-16.75%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jeinhaus
Copy link
Contributor Author

Jeinhaus commented Apr 8, 2024

I think we should make this optional (just as in flyte-binary) and probably leave the default behavior to be single service. wdyt?

Sounds good. I'll give it a shot.

@Jeinhaus Jeinhaus force-pushed the feature/add-separate-grpc-service-for-flyte-core branch from efd9226 to 8fd7240 Compare April 8, 2024 11:05
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Apr 8, 2024
@Jeinhaus
Create separate grpc service for flyteadmin
acd2f85 
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 <julian.einhaus@plan.one>
@Jeinhaus Jeinhaus force-pushed the feature/add-separate-grpc-service-for-flyte-core branch from 8fd7240 to 965a977 Compare April 8, 2024 11:06
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Apr 8, 2024
@Jeinhaus
Update documentation with new flyte-core chart default values
276679c 
Signed-off-by: Julian Einhaus <julian.einhaus@plan.one>
@Jeinhaus Jeinhaus force-pushed the feature/add-separate-grpc-service-for-flyte-core branch from 965a977 to 276679c Compare April 8, 2024 11:09
@Jeinhaus
Copy link
Contributor Author

Jeinhaus commented Apr 8, 2024

I think we should make this optional (just as in flyte-binary) and probably leave the default behavior to be single service. wdyt?

This got quite big, because I tried to make it kinda similar to the flyte-binary chart. wdyt?

davidmirror-ops
davidmirror-ops reviewed Apr 19, 2024
View reviewed changes
Copy link
Contributor

@davidmirror-ops davidmirror-ops left a comment

Choose a reason for hiding this comment

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

It's a big one for sure! Thanks for putting so much effort into this and sorry for the delay.
I think this also needs some local testing that I plan to complete once you review the suggestions.

Thanks

charts/flyte-core/templates/admin/service-grpc.yaml
- 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?

charts/flyte-core/templates/_helpers.tpl
{{- 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

charts/flyte-core/templates/admin/service.yaml
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?

charts/flyte-core/templates/common/ingress.yaml
{{- $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)

charts/flyte-core/values.yaml
@@ -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

@davidmirror-ops
Copy link
Contributor

@Jeinhaus any chance you can take a look at the comments? Thanks for putting so much effort into this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidmirror-ops davidmirror-ops davidmirror-ops left review comments

@neverett neverett Awaiting requested review from neverett

@ppiegaze ppiegaze Awaiting requested review from ppiegaze

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Jeinhaus @davidmirror-ops