-
Notifications
You must be signed in to change notification settings - Fork 743
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
Allow setting name of ports in generated services #113
Conversation
@tanordheim this looks ok to me, can you confirm that the metrics checks (success rate and request duration) are working with grpc? Also can you please open an issue for Kubernetes routing tests. Thanks |
I havent tested them extensively, all I've been able to do so far is just to have the deployments go out and have Flagger transition them into production using my canary settings. Let me try to produce some fake errors during transitioning in my GRPC services here to ensure that rollbacks happen as expected. |
The request duration should work. The success rate query is using this filter |
These are the internal GRPC status code; HTTP2 is still the transport underneath, which should mean that the error rates are correctly tracked by Istio by default. I'm doing some tests now to verify that though, give me 30 minutes :) |
Yeah, the response code things were problematic; it's tracked in Istio, but GRPC uses status code 200 for all responses; even request returning a GRPC error. This is likely something you'd have to instrument on your own though (we're using https://github.com/grpc-ecosystem/go-grpc-prometheus) and change the metric you're observing to be a GRPC specific metric. |
Flagger supports custom promql queries, so you could embedded the GRPC query in the canary spec. |
Yeah, I know, I just meant that it's maybe outside the scope of this PR to handle :) I can write some suggestions to the docs if you want to, though. |
Yes a GRPC note in the docs would be great. Thanks |
I added an example custom query with your pre-existing custom query example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @tanordheim
I'm taking Flagger for a spin (loving it), but I'm having some issues with our GRPC services that no longer work as expected when Flagger is managing the VirtualService/Service instances surrounding the services. This is namely due to the name of the ports in the generated Service objects hardcoded to be
http
, where in GRPCs case they should be set tohttp2
orgrpc
(orhttp2-something
orgrpc-something
in order for Envoy to configure itself as a HTTP2 proxy instead of a HTTP1-proxy.I did a quick change in the
CanaryService
API type and therouter.KubernetesRouter
in order to be able to specify aportName
in addition to aport
in my canary service definition; this solves my issue, but it might not be the way you guys would prefer to solve this - so I'm mostly opening up this PR to have a place to discuss the appropriate implementation. I couldn't find any tests that verified the Service creation (only retrieval/mapping) so this change is not covered by any tests.Fix: #115