flyte icon indicating copy to clipboard operation
flyte copied to clipboard

[flyte-core] Create separate grpc service for flyteadmin

Open Jeinhaus opened this issue 10 months ago • 6 comments

Tracking issue

https://github.com/flyteorg/flyte/issues/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?

Jeinhaus avatar Apr 03 '24 06:04 Jeinhaus

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).

welcome[bot] avatar Apr 03 '24 06:04 welcome[bot]

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

davidmirror-ops avatar Apr 03 '24 15:04 davidmirror-ops

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 05 '24 19:04 codecov[bot]

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 avatar Apr 08 '24 06:04 Jeinhaus

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?

Jeinhaus avatar Apr 08 '24 11:04 Jeinhaus

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

davidmirror-ops avatar May 09 '24 17:05 davidmirror-ops

This seems quite far and would be great to have! @Jeinhaus any chance you get to work on this still?

mruoss avatar Sep 16 '24 12:09 mruoss