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

Improve usability of gRPC probes in Kubernetes #33437

Merged
merged 1 commit into from
May 26, 2023

Conversation

Sgitario
Copy link
Contributor

Plus, I'm adding a new guide about the usage of gRPC services in Kubernetes.
Relates to #33219 (comment)

----

By default, the Quarkus Kubernetes will expose the HTTP port with name `http` which is bound to the Quarkus HTTP server, not the gRPC service. To expose the gRPC server instead, set the `quarkus.kubernetes.ingress.target-port=grpc` property in your application.properties:

Copy link
Member

Choose a reason for hiding this comment

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

How does that work with the new server where both http and gRPC are served on the same port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you share more details about this? But I guess that if both HTTP and gRPC are handing by the same port, it will work fine using the default http port name.
Though, if the port name grpc won't be generated any longer, we might need to revisit this statement.

Copy link
Member

Choose a reason for hiding this comment

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

With the old gRPC server, http and gRPC are served on different ports.
With the new gRPC server, HTTP and gRPC are served on the same port (which is very likely going to be https).

Both mode are supported at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

@alesj do you think we could add tests for both cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a deeper look into this that was implemented as part of #28654.
So, by default, it creates two servers: one for HTTP and another one for gRPC.
If quarkus.grpc.server.use-separate-server is false, then the gRPC port won't be listening.

Note that the grpc port is still bound to the generated Deployment/Service resources even though quarkus.grpc.server.use-separate-server is false. Because quarkus.grpc.server.use-separate-server is a runtime property, this might make sense. So, ultimately the user would need to select the target port (either http or grpc) of the generated resource.

We could again follow a best-effort approach, so if users use quarkus.grpc.server.use-separate-server=false at build time, we could remove the binding of the grpc port in the generated resources and use http by default as the target port. Wdyt @cescoffier ?

Copy link
Member

Choose a reason for hiding this comment

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

+1 or having an explicit property on the kubernetes side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have an explicit property on K8s side which is quarkus.kubernetes.ingress.target-port.
I'd prefer doing We could again follow a best-effort approach, so if users use quarkus.grpc.server.use-separate-server=false at build time, we could remove the binding of the grpc port in the generated resources and use http by default as the target port. as part of another pull request after merging this pull request.

So, if you're ok with the rest of the changes, can we proceed with merging this pull request?

docs/src/main/asciidoc/grpc-kubernetes.adoc Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented May 17, 2023

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

Overall, looks good I'd like us to change grpcActionEnabled thing. See comments for details.

Plus, I'm adding a new guide about the usage of gRPC services in Kubernetes.
Relates to quarkusio#33219 (comment)
@Sgitario Sgitario force-pushed the improve_grpc_probes branch from e47a6a3 to 81b09a8 Compare May 24, 2023 09:29
@Sgitario Sgitario requested review from cescoffier and iocanel May 24, 2023 09:30
@Sgitario
Copy link
Contributor Author

@cescoffier PR updated with your two change requests.
@iocanel my question still stands #33437 (comment)

@quarkus-bot
Copy link

quarkus-bot bot commented May 24, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

lgtm

@cescoffier cescoffier merged commit 4240f62 into quarkusio:main May 26, 2023
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone May 26, 2023
@Sgitario Sgitario deleted the improve_grpc_probes branch May 26, 2023 07:40
Sgitario added a commit to Sgitario/quarkus that referenced this pull request May 26, 2023
These changes will read the runtime property `quarkus.grpc.server.use-separate-server` and if false, it won't expose the gRPC port in the generated K8s deployment resources.

Relates quarkusio#33437 (comment)
michelle-purcell pushed a commit to michelle-purcell/quarkus that referenced this pull request May 29, 2023
These changes will read the runtime property `quarkus.grpc.server.use-separate-server` and if false, it won't expose the gRPC port in the generated K8s deployment resources.

Relates quarkusio#33437 (comment)
gsmet pushed a commit to gsmet/quarkus that referenced this pull request May 31, 2023
These changes will read the runtime property `quarkus.grpc.server.use-separate-server` and if false, it won't expose the gRPC port in the generated K8s deployment resources.

Relates quarkusio#33437 (comment)

(cherry picked from commit 4ccaea0)
sberyozkin pushed a commit to sberyozkin/quarkus that referenced this pull request Jun 21, 2023
These changes will read the runtime property `quarkus.grpc.server.use-separate-server` and if false, it won't expose the gRPC port in the generated K8s deployment resources.

Relates quarkusio#33437 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants