Shut down GRPC server gracefully #4238
Conversation
pchila
added
Team:Elastic-Agent-Control-Plane
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
|
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
|
Cancelled the build on the other PR because it was slowing down too much the CI run 😞 |
|
A simple change from Stop() to GracefulStop() seems to cause slowdown and failures in our integration tests. We probably need to fix other parts of the agent to make sure that we stop in time but still gracefully. |
pchila
force-pushed
the
stop-grpc-server-gracefully
branch
from
ffafeeb to
e26b61e
Compare
pchila
force-pushed
the
stop-grpc-server-gracefully
branch
from
e26b61e to
85ed162
Compare
|
0.0% Coverage on New Code
0.0% Duplication on New Code
| #pr: https://github.com/owner/repo/1234 | ||
|
|
||
| # Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). | ||
| # If not present is automatically filled by the tooling with the issue linked to the PR number. | ||
| #issue: https://github.com/owner/repo/1234 |
There was a problem hiding this comment.
Please add either the PR or the issue
| @@ -112,10 +112,18 @@ func (s *Server) Start() error { | |||
| // Stop stops the GRPC endpoint. | |||
| func (s *Server) Stop() { | |||
| if s.server != nil { | |||
| s.server.Stop() | |||
| s.logger.Info("Stopping GRPC server...") | |||
| s.server.GracefulStop() | |||
There was a problem hiding this comment.
perhaps there could be a timeout here, just to make sure we don't hang here forever. Unless there is a timeout on a upper layer. I just would like to avoid introduce something that might let the agent hanging during shutdown
|
@pchila it's been more than a month, any updates? |
No, not really and this is not among the priorities for the foreseeable future. I am going to close this and remove my assignment from the related issue so someone else can have a go. |
What does this PR do?
This PR changes the grpc server Stop() call to grpc.GracefulStop().
Why is it important?
This change is needed for the cases when there is a race between returning a result of an action and a shutdown/restart of agent. We have at least one such case for the
elastic-agent upgradecommand where sometimes the CLI reports an error such as EOF but the upgrade is successful: the agent just shut down the grpc server too quickly while restarting and the connection with the client gets closed before RPC result could be sent back (see #3890).Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files[ ] I have added tests that prove my fix is effective or that my feature works./changelog/fragmentsusing the changelog tool[ ] I have added an integration test or an E2E testAuthor's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs
Questions to ask yourself